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

[management] enable optional zitadel configuration of a PAT #3159

Merged
merged 2 commits into from
Jan 14, 2025

Conversation

adasauce
Copy link
Contributor

@adasauce adasauce commented Jan 8, 2025

for service user via the ExtraConfig fields.

re-opening PR #2661 after pull/rebase

Describe your changes

I'm sure this one is a little spicier of a take than my last PR to merge in some extra logging. During my struggle to get the recent zitadel working with netbird, I added the ability for netbird to just use a PAT to work around it while I investigated further.

Since the JWT and PAT both use the same mechanism to pass the token in the authorization header, the existing struct for JWTToken can be used and the authentication step short-circuited by supplying a long lasting AccessToken.

My current configuration looks somewhat like this:

    "IdpManagerConfig": {
        "ManagerType": "zitadel",
        "ClientConfig": {
           ...
        },
        "ExtraConfig": {
            "ManagementEndpoint": "https://zitadel-domain/management/v1",
            "PAT": "*****************************"
        },

As zitadel operators for more than just netbird, we have PATs for multiple different project service accounts across other orgs that integrate with it, and would prefer to use a PAT to simplify our security procedures too.

I'm not sure if this is the best end state for user experience as I rushed to implement something that worked in a fire, but figured this would be a good starting place to open the conversation.

Issue ticket number and link

Checklist

  • Is it a bug fix
  • Is a typo/documentation fix
  • Is a feature enhancement
  • It is a refactor
  • Created tests that fail without the change (if possible)
  • Extended the README / documentation, if necessary

@bcmmbaga
Copy link
Contributor

bcmmbaga commented Jan 8, 2025

Hi @adasauce, Could you please add a check for the existence of PAT when initializing the Zitadel manager. Also consider when PAT is set we should bypass the validation for other fields (ClientID, ClientSecret, TokenEndpoint, and GrantType) as they won't be required in this case.

if config.ClientID == "" {
return nil, fmt.Errorf("zitadel IdP configuration is incomplete, clientID is missing")
}
if config.ClientSecret == "" {
return nil, fmt.Errorf("zitadel IdP configuration is incomplete, ClientSecret is missing")
}
if config.TokenEndpoint == "" {
return nil, fmt.Errorf("zitadel IdP configuration is incomplete, TokenEndpoint is missing")
}
if config.ManagementEndpoint == "" {
return nil, fmt.Errorf("zitadel IdP configuration is incomplete, ManagementEndpoint is missing")
}
if config.GrantType == "" {
return nil, fmt.Errorf("zitadel IdP configuration is incomplete, GrantType is missing")
}

@adasauce
Copy link
Contributor Author

adasauce commented Jan 9, 2025

@bcmmbaga sure, i'll look into that asap.

@bcmmbaga bcmmbaga merged commit 0c28099 into netbirdio:main Jan 14, 2025
35 of 43 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants