(gcp_authn) add crypto utility for certificate fingerprinting#44956
Conversation
Signed-off-by: antoniovleonti <leonti@google.com>
Signed-off-by: antoniovleonti <leonti@google.com>
Signed-off-by: antoniovleonti <leonti@google.com>
|
Build failing because it doesn't meet the coverage requirements. I'll let the code owners here decide if we should adjust the requirement. |
|
/gemini review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a crypto_utils library for the GCP Authn filter to extract and validate certificate fingerprints. Review feedback highlights a logic issue where the current double-encoding method incorrectly encodes the '/' character; the reviewer suggests using absl::StrReplaceAll and updating the corresponding BUILD file and includes. Additionally, it is recommended to change the certificate chain logging level from info to debug to avoid excessive verbosity in production.
Signed-off-by: antoniovleonti <leonti@google.com>
Signed-off-by: antoniovleonti <leonti@google.com>
Signed-off-by: antoniovleonti <leonti@google.com>
Signed-off-by: antoniovleonti <leonti@google.com>
|
/assign @botengyao |
tyxia
left a comment
tyxia
left a comment
There was a problem hiding this comment.
Thanks for working on this. First pass
| } | ||
|
|
||
| bssl::UniquePtr<BIO> bio(BIO_new_mem_buf(file_content.data(), file_content.size())); | ||
| bssl::UniquePtr<X509> cert(PEM_read_bio_X509(bio.get(), nullptr, nullptr, nullptr)); |
There was a problem hiding this comment.
PEM_read_bio_X509 reads only the first certificate from the PEM. If certificate_chain contains a full chain (leaf + intermediates + roots), it is not guaranteed that first cert is the leaf cert.
There was a problem hiding this comment.
You are technically correct. However, I think it's standard practice to put the leaf cert first in a PEM file. In TlsContext::loadCertificateChain, it is also assumed the leaf cert is the first cert.
There was a problem hiding this comment.
could you add a comment to document this?
|
/wait |
tyxia
left a comment
tyxia
left a comment
There was a problem hiding this comment.
Another high level comment: Does this function need to sit inside of gcp_authn? Could be be a generally useful library in common place like common/tls/utility.cc etc?
Signed-off-by: antoniovleonti <leonti@google.com>
I considered this, but decided against it due to YAGNI & our quickly approaching deadline to have this code-complete. |
Signed-off-by: antoniovleonti <leonti@google.com>
Signed-off-by: antoniovleonti <leonti@google.com>
|
/retest |
Signed-off-by: antoniovleonti <leonti@google.com>
It is fine that we put it in gcp_authn due to dealine. My questions above is about whether this code will be YAGNI or not. Let's proceed as it is for now and think about moving the code later |
tyxia
left a comment
tyxia
left a comment
There was a problem hiding this comment.
LGTM, Thanks,
Please address the comment around documentation in a follow-up pr
4 participants
Commit Message: (gcp_authn) add crypto utility for certificate fingerprinting
Additional Description:
This PR introduces the crypto_utils.h library, which provides the functionality to extract and validate certificate fingerprints from a secret provider. This will later be used to generate bound tokens in this filter.
See also: #44929
Risk Level: none (not used yet)
Testing: new unit tests added
Docs Changes: none
Release Notes: none