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

fallback to env variable for oidc,oauth2 client id and secret #3217

Merged
merged 6 commits into from
Sep 5, 2024

Conversation

czhou-brex
Copy link
Contributor

@czhou-brex czhou-brex commented Sep 3, 2024

Similar to secureOauthTokenintrospectionAllKV, support falling back to environment varible for client id and secret for oidc and oauth2 filters, when the values passed in are empty strings.

This will allow compatibility with external secret stores.

Testing:

  • Run command line skipper with empty args for client_id and/or client_secret, in combination with those values set via env vars in oidc filter
  • Run command line skipper with empty args for client_id and/or client_secret, in combination with those values set via env vars in oauthgrant filter

Signed-off-by: Carl Zhou <czhou@brex.com>
@czhou-brex
Copy link
Contributor Author

Possibly closes #1952

@bjhaid
Copy link

bjhaid commented Sep 3, 2024

cc: @szuecs please help review 🙏

Signed-off-by: Carl Zhou <czhou@brex.com>
Signed-off-by: Carl Zhou <czhou@brex.com>
@czhou-brex czhou-brex marked this pull request as ready for review September 3, 2024 20:56
@MustafaSaber MustafaSaber added the minor no risk changes, for example new filters label Sep 4, 2024
@@ -242,19 +243,29 @@ func (s *tokenOidcSpec) CreateFilter(args []interface{}) (filters.Filter, error)
validity = defaultCookieValidity
}

oidcClientId := sargs[paramClientID]
if oidcClientId == "" {
oidcClientId, _ = os.LookupEnv("OIDC_CLIENT_ID")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should lookup once in skipper.go and pass the result values via OidcOptions

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

@czhou-brex czhou-brex Sep 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The existing OidcOptions struct does not have OIDC_Client_ID - should that be added to the field as a struct?
In addition would the arguments parsing in oidc.go have to be updated? https://github.com/zalando/skipper/blob/master/filters/auth/oidc.go#L80

Edit: I have added the two fields as structs and fetch the env variables in skipper.go

@szuecs
Copy link
Member

szuecs commented Sep 4, 2024

To close #1952 we would need to use secrets module and read from file to allow secrets rotation.

Signed-off-by: Carl Zhou <czhou@brex.com>
@szuecs
Copy link
Member

szuecs commented Sep 4, 2024

Changes look good. Please update the docs for the filters: see docs/reference/filters.md

table in https://opensource.zalando.com/skipper/reference/filters/#oauth2
listing in https://opensource.zalando.com/skipper/reference/filters/#openid-connect

For CLI flags we would override CLI values with the ENV, which should be written in help messages:
https://github.com/zalando/skipper/blob/master/config/config.go#L484-L485

Signed-off-by: Carl Zhou <czhou@brex.com>
Signed-off-by: Carl Zhou <czhou@brex.com>
@czhou-brex
Copy link
Contributor Author

Changes look good. Please update the docs for the filters: see docs/reference/filters.md

table in https://opensource.zalando.com/skipper/reference/filters/#oauth2 listing in https://opensource.zalando.com/skipper/reference/filters/#openid-connect

For CLI flags we would override CLI values with the ENV, which should be written in help messages: https://github.com/zalando/skipper/blob/master/config/config.go#L484-L485

Docs have been updated.

@szuecs
Copy link
Member

szuecs commented Sep 5, 2024

👍

2 similar comments
@RomanZavodskikh
Copy link
Contributor

👍

@MustafaSaber
Copy link
Member

👍

@MustafaSaber MustafaSaber merged commit d3e3756 into zalando:master Sep 5, 2024
10 checks passed
@MustafaSaber
Copy link
Member

thanks @czhou-brex for your contribution 🙏!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
minor no risk changes, for example new filters
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants