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

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

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.

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.

4 participants