Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

GitHub Proxy part 1: github integration resource #48999

Merged
merged 7 commits into from
Nov 23, 2024

Conversation

greedy52
Copy link
Contributor

@greedy52 greedy52 commented Nov 14, 2024

Part of:

Changes:

  • New github integration spec added to integration resource
  • Added WithSecrets to GetIntegration and ListIntegrations
  • Auth to populate per-app CA when creating github integration

Many files were touched but most are one-liner for the interface change.

tctl create example:

kind: integration
sub_kind: github
version: v1
metadata:
  name: github-my-org
spec:
  github:
    organization: my-org
  credentials:
    # oauth id and secret
    id_secret:
      id: "my-id"
      secret: "my-secret"

@greedy52 greedy52 added no-changelog Indicates that a PR does not require a changelog entry backport/branch/v17 labels Nov 14, 2024
@greedy52 greedy52 self-assigned this Nov 14, 2024
@greedy52 greedy52 marked this pull request as ready for review November 18, 2024 18:33
@github-actions github-actions bot added size/md tctl tctl - Teleport admin tool labels Nov 18, 2024
api/types/integration.go Outdated Show resolved Hide resolved
@rosstimothy
Copy link
Contributor

This doesn't follow the guidance on storing secrets from RFD 153.

If a resource has associated secrets (password, private key, jwt, mfa device, etc.) they should be defined in a separate resource and stored in a separate key range in the backend. The traditional pattern of defining secrets inline and only returning them if a with_secrets flag is provided causes a variety of problems and introduces opportunity for human error to accidentally include secrets when they should not have been. It would then be the responsibility of the caller to get both the base resource and the corresponding secret resource if required.

@greedy52
Copy link
Contributor Author

This doesn't follow the guidance on storing secrets from RFD 153.

If a resource has associated secrets (password, private key, jwt, mfa device, etc.) they should be defined in a separate resource and stored in a separate key range in the backend. The traditional pattern of defining secrets inline and only returning them if a with_secrets flag is provided causes a variety of problems and introduces opportunity for human error to accidentally include secrets when they should not have been. It would then be the responsibility of the caller to get both the base resource and the corresponding secret resource if required.

Ah, I see! Thanks for pointing out.

May I use plugin_static_credentials which seems implemented for this purpose? Some of the existing credential types align nicely with what's needed here.

// NewPluginStaticCredentialsService creates a new PluginStaticCredentialsService.
func NewPluginStaticCredentialsService(b backend.Backend) (*PluginStaticCredentialsService, error) {

Alternatively I can make a more generic static_credentials resource. What are your thoughts?

@r0mant
Copy link
Collaborator

r0mant commented Nov 19, 2024

@greedy52 Let's use plugin_static_credentials. They are specifically for storing secrets for hosted plugins and integrations.

@greedy52
Copy link
Contributor Author

@greedy52 Let's use plugin_static_credentials. They are specifically for storing secrets for hosted plugins and integrations.

Moved to static credentials now. PTAL.

@greedy52 greedy52 force-pushed the STeve/48762_integration branch from e447d53 to 1116e01 Compare November 22, 2024 03:45
@greedy52 greedy52 requested a review from GavinFrazar November 23, 2024 02:35
api/proto/teleport/legacy/types/types.proto Show resolved Hide resolved
api/types/integration.go Outdated Show resolved Hide resolved
api/types/integration.go Outdated Show resolved Hide resolved
api/types/integration.go Show resolved Hide resolved
api/types/integration.go Outdated Show resolved Hide resolved
api/types/integration.go Show resolved Hide resolved
lib/auth/integration/integrationv1/credentials.go Outdated Show resolved Hide resolved
lib/auth/integration/integrationv1/credentials.go Outdated Show resolved Hide resolved
lib/auth/integration/integrationv1/credentials.go Outdated Show resolved Hide resolved
@greedy52
Copy link
Contributor Author

Thanks for the quick reviews!

@greedy52 greedy52 enabled auto-merge November 23, 2024 18:38
@greedy52 greedy52 force-pushed the STeve/48762_integration branch from 431c09c to 2e63dc7 Compare November 23, 2024 19:04
@greedy52 greedy52 force-pushed the STeve/48762_integration branch from 2e63dc7 to 9a1609e Compare November 23, 2024 19:51
@greedy52 greedy52 added this pull request to the merge queue Nov 23, 2024
Merged via the queue into master with commit 3c6df87 Nov 23, 2024
43 checks passed
@greedy52 greedy52 deleted the STeve/48762_integration branch November 23, 2024 20:27
@public-teleport-github-review-bot

@greedy52 See the table below for backport results.

Branch Result
branch/v17 Failed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport/branch/v17 no-changelog Indicates that a PR does not require a changelog entry size/md tctl tctl - Teleport admin tool
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants