-
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?
Changes from all commits
ba295b7
a6b9b9a
e2aa451
c694c22
e5d0e66
d89298a
d53af8e
3f15f63
d76c8aa
27202d0
42d6f6a
f92c973
e81cae2
a62ab2f
5423714
cfe2424
0324e3c
cab45a2
3752193
defeea4
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,93 @@ | ||
| package com.yahoo.athenz.instance.provider.impl; | ||
|
|
||
| import com.nimbusds.jwt.proc.ConfigurableJWTProcessor; | ||
| import com.yahoo.athenz.auth.token.jwts.JwtsHelper; | ||
| import com.yahoo.athenz.auth.token.jwts.JwtsSigningKeyResolver; | ||
|
|
||
| import com.nimbusds.jose.proc.SecurityContext; | ||
|
|
||
| import java.util.HashMap; | ||
| import java.util.Map; | ||
| import java.util.Set; | ||
|
|
||
| public class InstanceGithubActionsProp { | ||
|
|
||
| private final Map<String, Prop> properties = new HashMap<>(); | ||
|
|
||
| // Inner class to hold property data | ||
| private static class Prop { | ||
| String audience; | ||
| String enterprise; | ||
| Set<String> dnsSuffixes; | ||
| String jwksUri; | ||
| ConfigurableJWTProcessor<SecurityContext> jwtProcessor; | ||
|
|
||
| Prop(String dnsSuffix, String audience, String enterprise, String jwksUri) { | ||
| dnsSuffixes = Set.of(dnsSuffix.split(",")); | ||
| this.audience = audience; | ||
| this.enterprise = enterprise; | ||
| this.jwksUri = jwksUri; | ||
| this.jwtProcessor = JwtsHelper.getJWTProcessor(new JwtsSigningKeyResolver(jwksUri, null)); | ||
| } | ||
| } | ||
|
|
||
| // No-argument constructor | ||
| public InstanceGithubActionsProp() { | ||
| } | ||
|
|
||
| // 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) { | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? |
||
| throw new IllegalArgumentException("One of the required properties is null"); | ||
| } | ||
| properties.put(issuer, new Prop(providerDnsSuffix, audience, enterprise, jwksUri)); | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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:
also please add a corresponding test case for it. |
||
| } | ||
|
|
||
| public Boolean hasIssuer(String issuer) { | ||
| if (issuer == null || issuer.isEmpty()) { | ||
| return false; | ||
| } | ||
| return properties.containsKey(issuer); | ||
| } | ||
|
|
||
| // Getter methods | ||
| public Set<String> getDnsSuffixes(String issuer) { | ||
| if (!properties.containsKey(issuer)) { | ||
| return null; | ||
| } | ||
| return properties.get(issuer).dnsSuffixes; | ||
| } | ||
|
|
||
| public String getAudience(String issuer) { | ||
| if (!properties.containsKey(issuer)) { | ||
| return null; | ||
| } | ||
| return properties.get(issuer).audience; | ||
| } | ||
|
|
||
| public String getEnterprise(String issuer) { | ||
| if (!properties.containsKey(issuer)) { | ||
| return null; | ||
| } | ||
| return properties.get(issuer).enterprise; | ||
| } | ||
|
|
||
| public Boolean hasEnterprise (String issuer) { | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. please remove the space after the method name |
||
| String enterprise = getEnterprise(issuer); | ||
| return enterprise != null && !enterprise.isEmpty(); | ||
| } | ||
|
|
||
| public String getJwksUri(String issuer) { | ||
| if (!properties.containsKey(issuer)) { | ||
| return null; | ||
| } | ||
| return properties.get(issuer).jwksUri; | ||
| } | ||
|
|
||
| public ConfigurableJWTProcessor<SecurityContext> getJwtProcessor(String issuer) { | ||
| if (!properties.containsKey(issuer)) { | ||
| return null; | ||
| } | ||
| return properties.get(issuer).jwtProcessor; | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -15,15 +15,17 @@ | |
| */ | ||
| package com.yahoo.athenz.instance.provider.impl; | ||
|
|
||
| import com.fasterxml.jackson.databind.ObjectMapper; | ||
| import com.fasterxml.jackson.core.type.TypeReference; | ||
| import com.nimbusds.jose.proc.SecurityContext; | ||
| import com.nimbusds.jwt.JWTClaimsSet; | ||
| import com.nimbusds.jwt.SignedJWT; | ||
| import com.nimbusds.jwt.proc.ConfigurableJWTProcessor; | ||
| import com.yahoo.athenz.auth.Authorizer; | ||
| import com.yahoo.athenz.auth.KeyStore; | ||
| import com.yahoo.athenz.auth.Principal; | ||
| import com.yahoo.athenz.auth.impl.SimplePrincipal; | ||
| import com.yahoo.athenz.auth.token.jwts.JwtsHelper; | ||
| import com.yahoo.athenz.auth.token.jwts.JwtsSigningKeyResolver; | ||
| import com.yahoo.athenz.common.server.util.config.dynamic.DynamicConfigLong; | ||
| import com.yahoo.athenz.instance.provider.InstanceConfirmation; | ||
| import com.yahoo.athenz.instance.provider.InstanceProvider; | ||
|
|
@@ -33,8 +35,13 @@ | |
| import org.slf4j.LoggerFactory; | ||
|
|
||
| import javax.net.ssl.SSLContext; | ||
|
|
||
| import java.io.IOException; | ||
| import java.nio.file.Paths; | ||
| import java.util.*; | ||
| import java.util.concurrent.TimeUnit; | ||
| import java.nio.file.Files; | ||
| import java.nio.file.Path; | ||
|
|
||
| import static com.yahoo.athenz.common.server.util.config.ConfigManagerSingleton.CONFIG_MANAGER; | ||
|
|
||
|
|
@@ -45,13 +52,21 @@ public class InstanceGithubActionsProvider implements InstanceProvider { | |
| private static final String URI_INSTANCE_ID_PREFIX = "athenz://instanceid/"; | ||
| private static final String URI_SPIFFE_PREFIX = "spiffe://"; | ||
|
|
||
| static final String GITHUB_ACTIONS_PROP_PROVIDER_DNS_SUFFIX = "athenz.zts.github_actions.provider_dns_suffix"; | ||
| static final String KEY_PROVIDER_DNS_SUFFIX = "provider_dns_suffix"; | ||
| static final String KEY_AUDIENCE = "audience"; | ||
| static final String KEY_ENTERPRISE = "enterprise"; | ||
| static final String KEY_JWKS_URI = "jwks_uri"; | ||
| static final String KEY_ISSUER = "issuer"; | ||
| static final String KEY_JWK_PROCESSOR = "jwk_processor"; | ||
|
|
||
| static final String GITHUB_ACTIONS_PROP_FILE_PATH = "athenz.zts.github_actions.prop_file_path"; | ||
| static final String GITHUB_ACTIONS_PROP_PROVIDER_DNS_SUFFIX = "athenz.zts.github_actions." + KEY_PROVIDER_DNS_SUFFIX; | ||
| static final String GITHUB_ACTIONS_PROP_BOOT_TIME_OFFSET = "athenz.zts.github_actions.boot_time_offset"; | ||
| static final String GITHUB_ACTIONS_PROP_CERT_EXPIRY_TIME = "athenz.zts.github_actions.cert_expiry_time"; | ||
| static final String GITHUB_ACTIONS_PROP_ENTERPRISE = "athenz.zts.github_actions.enterprise"; | ||
| static final String GITHUB_ACTIONS_PROP_AUDIENCE = "athenz.zts.github_actions.audience"; | ||
| static final String GITHUB_ACTIONS_PROP_ISSUER = "athenz.zts.github_actions.issuer"; | ||
| static final String GITHUB_ACTIONS_PROP_JWKS_URI = "athenz.zts.github_actions.jwks_uri"; | ||
| static final String GITHUB_ACTIONS_PROP_ENTERPRISE = "athenz.zts.github_actions." + KEY_ENTERPRISE; | ||
| static final String GITHUB_ACTIONS_PROP_AUDIENCE = "athenz.zts.github_actions." + KEY_AUDIENCE; | ||
| static final String GITHUB_ACTIONS_PROP_ISSUER = "athenz.zts.github_actions." + KEY_ISSUER; | ||
| static final String GITHUB_ACTIONS_PROP_JWKS_URI = "athenz.zts.github_actions." + KEY_JWKS_URI; | ||
|
|
||
| static final String GITHUB_ACTIONS_ISSUER = "https://token.actions.githubusercontent.com"; | ||
| static final String GITHUB_ACTIONS_ISSUER_JWKS_URI = "https://token.actions.githubusercontent.com/.well-known/jwks"; | ||
|
|
@@ -61,12 +76,8 @@ public class InstanceGithubActionsProvider implements InstanceProvider { | |
| public static final String CLAIM_EVENT_NAME = "event_name"; | ||
| public static final String CLAIM_REPOSITORY = "repository"; | ||
|
|
||
| Set<String> dnsSuffixes = null; | ||
| String githubIssuer = null; | ||
| InstanceGithubActionsProp props = null; | ||
| String provider = null; | ||
| String audience = null; | ||
| String enterprise = null; | ||
| ConfigurableJWTProcessor<SecurityContext> jwtProcessor = null; | ||
| Authorizer authorizer = null; | ||
| DynamicConfigLong bootTimeOffsetSeconds; | ||
| long certExpiryTime; | ||
|
|
@@ -76,49 +87,81 @@ public Scheme getProviderScheme() { | |
| return Scheme.CLASS; | ||
| } | ||
|
|
||
| public void initializeFromFilePath() throws ProviderResourceException { | ||
| final String propFilePath = System.getProperty(GITHUB_ACTIONS_PROP_FILE_PATH, ""); | ||
| if (StringUtil.isEmpty(propFilePath)) { | ||
| return; // no prop file path specified, nothing to do | ||
| } | ||
|
|
||
| Path path = Paths.get(propFilePath); | ||
| try { | ||
| Map<String, List<Map<String, Object>>> propJson = new ObjectMapper().readValue( | ||
| Files.readAllBytes(path), | ||
| new TypeReference<Map<String, List<Map<String, Object>>>>() { } | ||
| ); | ||
|
|
||
| for (Map<String, Object> prop : propJson.get("props")) { | ||
| String issuer = (String) prop.get(KEY_ISSUER); | ||
| if (StringUtil.isEmpty(issuer)) { | ||
| throw forbiddenError("Missing required issuer prop file: " + propFilePath); | ||
| } | ||
|
|
||
| props.addProperties( | ||
| issuer, | ||
| (String) prop.get(KEY_PROVIDER_DNS_SUFFIX), | ||
| (String) prop.get(KEY_AUDIENCE), | ||
| (String) prop.get(KEY_ENTERPRISE), | ||
| extractGitHubIssuerJwksUri(issuer, (String) prop.get(KEY_JWKS_URI)) | ||
| ); | ||
| } | ||
|
|
||
| } catch (IOException ex) { | ||
| throw forbiddenError("Unable to parse jwk endpoints file: " + propFilePath | ||
| + ", error: " + ex.getMessage()); | ||
| } | ||
| } | ||
|
|
||
| @Override | ||
| public void initialize(String provider, String providerEndpoint, SSLContext sslContext, | ||
| KeyStore keyStore) { | ||
|
|
||
| props = new InstanceGithubActionsProp(); | ||
|
|
||
| // save our provider name | ||
|
|
||
| this.provider = provider; | ||
|
|
||
| // lookup the zts audience. if not specified we'll default to athenz.io | ||
|
|
||
| audience = System.getProperty(GITHUB_ACTIONS_PROP_AUDIENCE, "athenz.io"); | ||
|
|
||
| // determine the dns suffix. if this is not specified we'll just default to github-actions.athenz.cloud | ||
|
|
||
| final String dnsSuffix = System.getProperty(GITHUB_ACTIONS_PROP_PROVIDER_DNS_SUFFIX, "github-actions.athenz.io"); | ||
| dnsSuffixes = new HashSet<>(); | ||
| dnsSuffixes.addAll(Arrays.asList(dnsSuffix.split(","))); | ||
|
|
||
| // how long the instance must be booted in the past before we | ||
| // stop validating the instance requests | ||
|
|
||
| long timeout = TimeUnit.SECONDS.convert(5, TimeUnit.MINUTES); | ||
| bootTimeOffsetSeconds = new DynamicConfigLong(CONFIG_MANAGER, GITHUB_ACTIONS_PROP_BOOT_TIME_OFFSET, timeout); | ||
|
|
||
| // determine if we're running in enterprise mode | ||
|
|
||
| enterprise = System.getProperty(GITHUB_ACTIONS_PROP_ENTERPRISE); | ||
|
|
||
| // get default/max expiry time for any generated tokens - 6 hours | ||
|
|
||
| certExpiryTime = Long.parseLong(System.getProperty(GITHUB_ACTIONS_PROP_CERT_EXPIRY_TIME, "360")); | ||
|
|
||
| // initialize our jwt processor | ||
| String githubIssuer = System.getProperty(GITHUB_ACTIONS_PROP_ISSUER, GITHUB_ACTIONS_ISSUER); | ||
|
|
||
| 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 | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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). |
||
| System.getProperty(GITHUB_ACTIONS_PROP_AUDIENCE, "athenz.io"), // lookup the zts audience. if not specified we'll default to athenz.io | ||
| System.getProperty(GITHUB_ACTIONS_PROP_ENTERPRISE), // determine if we're running in enterprise mode | ||
| extractGitHubIssuerJwksUri(githubIssuer, System.getProperty(GITHUB_ACTIONS_PROP_JWKS_URI)) | ||
| ); | ||
|
|
||
| githubIssuer = System.getProperty(GITHUB_ACTIONS_PROP_ISSUER, GITHUB_ACTIONS_ISSUER); | ||
| jwtProcessor = JwtsHelper.getJWTProcessor(new JwtsSigningKeyResolver(extractGitHubIssuerJwksUri(githubIssuer), null)); | ||
| try { | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 initializeFromFilePath(); // at this point if our configuration is set as in we have valid if (!props.isEmpty()) { // otherwise add a new issuer entry based on the configured property values String githubIssuer = System.getProperty(GITHUB_ACTIONS_PROP_ISSUER, GITHUB_ACTIONS_ISSUER); So with this model there is only a single way to configure the provider - either valid file config or system properties.
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
||
| initializeFromFilePath(); // initialize from file path if specified. If not specified, nothing happens. | ||
| } catch (ProviderResourceException ex) { | ||
| LOGGER.error("Unable to initialize from file path: {}", ex.getMessage()); | ||
| } | ||
| } | ||
|
|
||
| String extractGitHubIssuerJwksUri(final String issuer) { | ||
| String extractGitHubIssuerJwksUri(final String issuer, String jwksUri) { | ||
|
|
||
| // if we have the value configured then that's what we're going to use | ||
|
|
||
| String jwksUri = System.getProperty(GITHUB_ACTIONS_PROP_JWKS_URI); | ||
| if (!StringUtil.isEmpty(jwksUri)) { | ||
| return jwksUri; | ||
| } | ||
|
|
@@ -187,17 +230,26 @@ public InstanceConfirmation confirmInstance(InstanceConfirmation confirmation) t | |
| } | ||
|
|
||
| StringBuilder errMsg = new StringBuilder(256); | ||
| String claimIssuer = null; | ||
| try { | ||
| // parse the token and get the issuer claim | ||
| claimIssuer = SignedJWT.parse(attestationData).getJWTClaimsSet().getIssuer(); | ||
| } catch (Exception ex) { | ||
| errMsg.append("Unable to parse token: ").append(ex.getMessage()); | ||
| throw forbiddenError("Unable to parse token: " + ex.getMessage()); | ||
| } | ||
|
|
||
| final String reqInstanceId = InstanceUtils.getInstanceProperty(instanceAttributes, | ||
| InstanceProvider.ZTS_INSTANCE_ID); | ||
| if (!validateOIDCToken(attestationData, instanceDomain, instanceService, reqInstanceId, errMsg)) { | ||
| throw forbiddenError("Unable to validate Certificate Request: " + errMsg); | ||
| if (!validateOIDCToken(claimIssuer, attestationData, instanceDomain, instanceService, reqInstanceId, errMsg)) { | ||
| throw forbiddenError("Unable to validate Certificate Request: " + errMsg); | ||
| } | ||
|
|
||
| // validate the certificate san DNS names | ||
|
|
||
| StringBuilder instanceId = new StringBuilder(256); | ||
| if (!InstanceUtils.validateCertRequestSanDnsNames(instanceAttributes, instanceDomain, | ||
| instanceService, dnsSuffixes, null, null, false, instanceId, null)) { | ||
| instanceService, props.getDnsSuffixes(claimIssuer), null, null, false, instanceId, null)) { | ||
| throw forbiddenError("Unable to validate certificate request sanDNS entries"); | ||
| } | ||
|
|
||
|
|
@@ -245,11 +297,16 @@ boolean validateSanUri(final String sanUri) { | |
| return true; | ||
| } | ||
|
|
||
| boolean validateOIDCToken(final String jwToken, final String domainName, final String serviceName, | ||
| boolean validateOIDCToken(final String claimIssuer, final String jwToken, final String domainName, final String serviceName, | ||
| final String instanceId, StringBuilder errMsg) { | ||
| if (props == null) { | ||
| errMsg.append("InstanceGithubActionsProp not initialized"); | ||
| return false; | ||
| } | ||
|
|
||
| ConfigurableJWTProcessor<SecurityContext> jwtProcessor = props.getJwtProcessor(claimIssuer); | ||
| if (jwtProcessor == null) { | ||
| errMsg.append("JWT Processor not initialized"); | ||
| errMsg.append("JWT Processor not found for issuer: ").append(claimIssuer); | ||
| return false; | ||
| } | ||
|
|
||
|
|
@@ -261,25 +318,15 @@ boolean validateOIDCToken(final String jwToken, final String domainName, final S | |
| return false; | ||
| } | ||
|
|
||
| // verify the issuer in set to GitHub Actions | ||
|
|
||
| if (!githubIssuer.equals(claimsSet.getIssuer())) { | ||
| errMsg.append("token issuer is not GitHub Actions: ").append(claimsSet.getIssuer()); | ||
| return false; | ||
| } | ||
|
|
||
| // verify that token audience is set for our service | ||
|
|
||
| if (!audience.equals(JwtsHelper.getAudience(claimsSet))) { | ||
| if (!props.getAudience(claimIssuer).equals(JwtsHelper.getAudience(claimsSet))) { | ||
| errMsg.append("token audience is not ZTS Server audience: ").append(JwtsHelper.getAudience(claimsSet)); | ||
| return false; | ||
| } | ||
|
|
||
| // verify that token issuer is set for our enterprise if one is configured | ||
|
|
||
| if (!StringUtil.isEmpty(enterprise)) { | ||
| if (props.hasEnterprise(claimIssuer)) { | ||
| final String tokenEnterprise = JwtsHelper.getStringClaim(claimsSet, CLAIM_ENTERPRISE); | ||
| if (!enterprise.equals(tokenEnterprise)) { | ||
| if (!props.getEnterprise(claimIssuer).equals(tokenEnterprise)) { | ||
| errMsg.append("token enterprise is not the configured enterprise: ").append(tokenEnterprise); | ||
| return false; | ||
| } | ||
|
|
||
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)