-
Notifications
You must be signed in to change notification settings - Fork 298
Feat: ConfigFiles for Multiple GitHub Action Provider Environments #2992
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
base: master
Are you sure you want to change the base?
Conversation
c06f064 to
2a3a938
Compare
Signed-off-by: Jeongwoo Kim - jekim <[email protected]>
Signed-off-by: Jeongwoo Kim - jekim <[email protected]>
Signed-off-by: Jeongwoo Kim - jekim <[email protected]>
Signed-off-by: Jeongwoo Kim - jekim <[email protected]>
Signed-off-by: Jeongwoo Kim - jekim <[email protected]>
Signed-off-by: Jeongwoo Kim - jekim <[email protected]>
Signed-off-by: Jeongwoo Kim - jekim <[email protected]>
Signed-off-by: Jeongwoo Kim - jekim <[email protected]>
Signed-off-by: Jeongwoo Kim - jekim <[email protected]>
Signed-off-by: Jeongwoo Kim - jekim <[email protected]>
Signed-off-by: Jeongwoo Kim - jekim <[email protected]>
Signed-off-by: Jeongwoo Kim - jekim <[email protected]>
Signed-off-by: Jeongwoo Kim - jekim <[email protected]>
Signed-off-by: Jeongwoo Kim - jekim <[email protected]>
Signed-off-by: Jeongwoo Kim - jekim <[email protected]>
Signed-off-by: Jeongwoo Kim - jekim <[email protected]>
Signed-off-by: Jeongwoo Kim - jekim <[email protected]>
Signed-off-by: Jeongwoo Kim - jekim <[email protected]>
Signed-off-by: Jeongwoo Kim - jekim <[email protected]>
Signed-off-by: Jeongwoo Kim - jekim <[email protected]>
2a3a938 to
defeea4
Compare
| @@ -0,0 +1,93 @@ | |||
| package com.yahoo.athenz.instance.provider.impl; | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please add the Athenz Authors header block (see other files in the repo)
| if (issuer == null || providerDnsSuffix == null || audience == null || jwksUri == null) { | ||
| throw new IllegalArgumentException("One of the required properties is null"); | ||
| } | ||
| properties.put(issuer, new Prop(providerDnsSuffix, audience, enterprise, jwksUri)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should add a check to see if the issuer is already set or not. otherwise, if the admin makes a mistake and specifies the same issuer in two different blocks, then the second one will override the first one and there would be no indication that it has taken place until the end users complain about their actions not working. e.g. both blocks have the same issuer:
{ "props": [ { "provider_dns_suffix": "example-suffix1,example-suffix2", "enterprise": "enterprise1", "audience": "https://audience1.com", "issuer": "https://issuer1.com", "jwks_uri": "https://issuer1.com/jwks" }, { "provider_dns_suffix": "example-suffix3,example-suffix4", "enterprise": "enterprise2", "audience": "https://audience2.com", "issuer": "https://issuer1.com", "jwks_uri": "https://issuer2.com/jwks" } ] }
also please add a corresponding test case for it.
|
|
||
| // Method to add properties | ||
| public void addProperties(String issuer, String providerDnsSuffix, String audience, String enterprise, String jwksUri) { | ||
| if (issuer == null || providerDnsSuffix == null || audience == null || jwksUri == null) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we only care about nulls or should we also check for empty strings?
| return properties.get(issuer).enterprise; | ||
| } | ||
|
|
||
| public Boolean hasEnterprise (String issuer) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please remove the space after the method name
|
|
||
| props.addProperties( | ||
| githubIssuer, | ||
| System.getProperty(GITHUB_ACTIONS_PROP_PROVIDER_DNS_SUFFIX, "github-actions.athenz.io"), // determine the dns suffix. if this is not specified we'll just default to github-actions.athenz.cloud |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's fix the comment to say githib-actions.athenz.io instead of athenz.cloud (the original code had the issue).
|
|
||
| githubIssuer = System.getProperty(GITHUB_ACTIONS_PROP_ISSUER, GITHUB_ACTIONS_ISSUER); | ||
| jwtProcessor = JwtsHelper.getJWTProcessor(new JwtsSigningKeyResolver(extractGitHubIssuerJwksUri(githubIssuer), null)); | ||
| try { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure if I like the order of objects here since it kind of creates a somewhat ambiguous set up. I can configure both a file and property settings and they both will be set accordingly which is probably not what we expect. I'd like to have the following order where the preference is given to the file config and the property settings are read only if the file config is not set. So we should have the following logic:
// we should not catch any exceptions and instead any errors
// need to be reported to the caller as failures
initializeFromFilePath();
// at this point if our configuration is set as in we have valid
// entries in our props objects, we're done and nothing else to do
if (!props.isEmpty()) {
return;
}
// otherwise add a new issuer entry based on the configured property values
String githubIssuer = System.getProperty(GITHUB_ACTIONS_PROP_ISSUER, GITHUB_ACTIONS_ISSUER);
.... (like you have the code above).
So with this model there is only a single way to configure the provider - either valid file config or system properties.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So just to follow-up, in the InstanceGithubActionsProp class you need to define a new member method called isEmpty that returns boolean whether or not member field properties hashmap has entries or not.
Background
The current GItHubActionInstanceProvider does not allow multiple GitHub Action environments with one Athenz ZTS Server.
What's done?
Allows ZTS owner to set multiple GitHub Action environments with config, with
issueras an IDBy setting following:
With the following:
{ "props": [ { "provider_dns_suffix": "example-suffix1", "enterprise": "enterprise1", "audience": "https://audience1.com", "issuer": "https://issuer1.com", "jwks_uri": "https://issuer1.com/jwks" }, { "provider_dns_suffix": "example-suffix3,example-suffix4", "enterprise": "enterprise2", "audience": "https://audience2.com", "issuer": "https://issuer2.com", "jwks_uri": "https://issuer2.com/jwks" } ] }Minor Changes
InstanceGithubActionsProp not initializedaddedJWT Processor not initializedlog modifiedJWT Processor not found for issuer: <issuer_name>token issuer is not GitHub Actions: <claim_issuer>removedContribution Checklist:
Attach Screenshots (Optional)