GCF support for Java #2209
GCF support for Java #2209
Conversation
Signed-off-by: gilad.bendor <gilad.bendor@yahooinc.com>
| new DERIA5String(athenzService + '.' + athenzDomain.replace('.', '-') + '.' + certDomain)), | ||
| new GeneralName( | ||
| GeneralName.dNSName, | ||
| new DERIA5String("gcf-" + gcpProjectId + '-' + athenzService + ".instanceid.athenz." + certDomain)), // TODO: Not sure about this |
There was a problem hiding this comment.
we should no longer use this sanDns entry. instead we we should use sanUri to specify the instance id.
we need to be careful of the order as the spiffe URI must be the first uri in the cert. So add the following entry at the end of this block:
athenz://instanceid/%s/gcp-function-%s", provider, gcpProjectId
There was a problem hiding this comment.
Here is the current code:
return new GeneralName[]{
new GeneralName(
GeneralName.dNSName,
new DERIA5String(athenzService + '.' + athenzDomain.replace('.', '-') + '.' + certDomain)),
new GeneralName(
GeneralName.dNSName,
new DERIA5String("gcf-" + gcpProjectId + '-' + athenzService + ".instanceid.athenz." + certDomain)), // TODO: Not sure about this
new GeneralName(
GeneralName.uniformResourceIdentifier,
new DERIA5String("spiffe://" + athenzDomain + "/sa/" + athenzService)),
};
Here is the best I can come up with (provider = "sys.gcp." + gcpRegion):
return new GeneralName[]{
new GeneralName(
GeneralName.uniformResourceIdentifier,
new DERIA5String("spiffe://" + athenzDomain + "/sa/" + athenzService)),
new GeneralName(
GeneralName.uniformResourceIdentifier,
new DERIA5String("athenz://instanceid/" + provider + "/gcp-function-" + gcpProjectId)),
};
Can you confirm?
|
|
||
| /** Used to parse ZTS response */ | ||
| @JsonIgnoreProperties(ignoreUnknown = true) | ||
| private static class InstanceIdentity { |
There was a problem hiding this comment.
any reason why we have this class here? we already have all the classes defined in athenz-zts-core. So let's use com.yahoo.athenz.zts.InstanceIdentity instead of defining this class here.
| public String x509CertificateSigner; | ||
| } | ||
|
|
||
| static String ATTESTATION_DATA_URL_PREFIX = "http://metadata/computeMetadata/v1/instance/service-accounts/default/identity?format=full&audience="; |
There was a problem hiding this comment.
let's be consistent with all other classes in the project. define these 3 variables at the top of the class definition and not at the bottom.
| static String ATTESTATION_DATA_URL_PREFIX = "http://metadata/computeMetadata/v1/instance/service-accounts/default/identity?format=full&audience="; | ||
|
|
||
| private static final ObjectMapper OBJECT_MAPPER = new ObjectMapper(); | ||
| private static final Logger LOG = LoggerFactory.getLogger(MethodHandles.lookup().lookupClass()); |
There was a problem hiding this comment.
we have this LOG object defined yet all of the log statements are commented out. If we're not planning any log entries, let's remove this and the dependency otherwise, let's enable the log statements. Be careful about the identity token ones - let's make sure we don't have any tokens logged even in debug mode.
There was a problem hiding this comment.
...all logs reluctantly removed... ;-)
| .build()) { | ||
|
|
||
| // Construct an HTTP POST request. | ||
| String postPayload = "{" + |
There was a problem hiding this comment.
let's not generate the payload manually by generating the json here. We already have a model classes defined in athenz-zts-core.
Let's define a com.yahoo.athenz.zts.InstanceRegisterInformation object, set the values in the object and then we can convert that object into json or I believe there are other Entity object setters.
| * @param gcpProjectId GCP project-id that the function runs in | ||
| * @param athenzProvider name of the provider service for GCP Cloud-Functions | ||
| * @param ztsUrl Something like: https://...:.../zts/v1 | ||
| * @param certDomain TODO: Abhijeet - explain what this is... |
There was a problem hiding this comment.
certDomain - string identifying the dns domain for generating SAN fields. for example, for a domain with sports and service api and certdomain athenz.io, the sanDNS entry in the certificate will be set to api.sports.athenz.io
| * @param optionalLocality Optional field in the certificate's Subject. | ||
| * @param optionalOrganization Optional field in the certificate's Subject. | ||
| * @param optionalOrganizationUnit Optional field in the certificate's Subject. | ||
| * @return GCPFunctionIdentity with private key and certificate |
There was a problem hiding this comment.
it returns PrivateAndCertificate and not GCPFunctionIdentity
| public static int ATTESTATION_READ_TIMEOUT_MS = ZTS_READ_TIMEOUT_MS; | ||
|
|
||
| /** Response of {@link #getGCPFunctionServiceCertificate} */ | ||
| public static class PrivateAndCertificate { |
There was a problem hiding this comment.
golang uses X509KeyPair as the object name that includes the private key and certificate so how about using that instead of PrivateAndCertificate.
|
|
||
| // Build the certificate's Subject fields - as a single string. | ||
| // At the end, certDn would look something like this: "c=US, s=CA, ou=Eng" | ||
| // Build the certificate's Subject fields - as a single string. |
| String athenzProvider, | ||
| String ztsUrl, | ||
| String certDomain, | ||
| String optionalCountry, |
There was a problem hiding this comment.
we should not include optional in the field names to identity the field as optional - these are already included in the javadocs. let's just include rdn instead of optional to identity that these are relative distinguished name components - e.g. rdnCountry, rdnState, etc.
Signed-off-by: gilad.bendor <gilad.bendor@yahooinc.com>
No description provided.