-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
[refactor] - GCP detector #2647
base: main
Are you sure you want to change the base?
Conversation
Wouldn't the API call still be necessary? A genuine key does not necessarily equate to a valid key (e.g., revocation.) |
I totally understand your concern about the necessity of making an API call to verify the validity of the credential, considering factors like revocation, I had the same concerns initially. I have actually tested this scenario by disabling the service account, and I can confirm that the approach of comparing the public keys works as expected. When a service account is expired or disabled, the request to fetch the certificate from the ClientX509CertURL returns a 404 status code. This indicates that the certificate is no longer accessible, and the service account is correctly considered invalid. By relying on the availability of the certificate at the ClientX509CertURL, we can effectively validate the status of the service account. If the certificate is not found (404 status code), it means the service account is no longer valid, regardless of the public key comparison. This approach aligns with the recommendation provided to us by the Google team. They suggested that if we can successfully retrieve the certificate and find a matching public key corresponding to the private key in the credential, it can be considered live and verified. Let ma update the comments to document that. |
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 think I see a way to support indeterminacy (commented inline) - is that possible?
(And out of curiosity, what prompted this change?)
The team at Google requested we use this approach instead. |
} | ||
|
||
cert, ok := resp[privKeyID] | ||
return cert, ok, nil |
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.
Are both string
and bool
return types necessary? If bool
is only true
when string != ""
, you could return (string, error)
and just check whether string != ""
.
(Idk, the three return values confused me while reading this.)
} | ||
|
||
// extractPublicKeyFromCert decodes the PEM-encoded certificate to extract the RSA public key. | ||
func extractPublicKeyFromCert(certPEM string) (*rsa.PublicKey, bool) { |
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.
Is it worth handling these errors? The input is coming from a trusted API in a known format; I'd think that any failures would be highly unusual and something worth looking at.
return false, nil | ||
} | ||
|
||
privateKey, ok := extractPrivateKey(key.PrivateKey) |
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.
Would it make sense to run this prior to the verify
block? That way, if the key isn't valid (e.g., ---- BEGIN PRIVATE KEY ----- \n<YOUR-KEY-HERE>\n---- END PRIVATE KEY -----
) it doesn't need to make a network request. It would also filter out invalid results if verification is disabled/unverified results are being shown; right now, there is no false positive check.
@@ -90,41 +103,144 @@ func (s Scanner) FromData(ctx context.Context, verify bool, data []byte) (result | |||
|
|||
credBytes, _ := json.Marshal(creds) |
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 need to actually capture the whole json blob and marshal the json? One issue with GCP keys is they're so big sometimes they get cut off in scanning windows. Maybe now that we've updated verifier logic, we could get away with just a capture group around the private key, and a capture group around the x509 URL
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.
Technically all that's required is the private_key
and client_email
(name@project_id.iam.gserviceaccount.com
). The X509 URL is just https://www.googleapis.com/robot/v1/metadata/x509/$client_email
.
However, the logic would need to be updated to check all public keys and not the specific key ID.
{
"798a44fefb45a6c4b78c3e4e0accecc599cc7f0e": "-----BEGIN CERTIFICATE-----\nMIIDTDCCAjSgAwIBAgIIFU53DH3Qt0wwDQYJKoZIhvcNAQEFBQAwSTFHMEUGA1UE\nAww+aWFwLTE4MC5hcGktODUwNzc3OTQ0Njg5ODM3MDY4NS03MzIzODcuaWFtLmdz\nZXJ2aWNlYWNjb3VudC5jb20wHhcNMjQwNjE4MjEwNDIyWhcNMjQwNzA1MDkxOTIy\nWjBJMUcwRQYDVQQDDD5pYXAtMTgwLmFwaS04NTA3Nzc5NDQ2ODk4MzcwNjg1LTcz\nMjM4Ny5pYW0uZ3NlcnZpY2VhY2NvdW50LmNvbTCCASIwDQYJKoZIhvcNAQEBBQAD\nggEPADCCAQoCggEBAKuGOcFgUAMx+f4TW5U9JzNxk9KFA8hiOHZV8AC05Uk4kaSl\nEhGPgeXCZsyifBh/yEOz/pHcaPLcWLO3DCjXp4tIWuFiuav1n90FVRaWek+nf99J\nuwF5J2UBGzOq5U03+Mw2h5+MZPpceE4O093AUJiWzBXV92SDx7ZWoHLeorMCqmJa\nCsEmj1TYLFCnl7aUf/JKkRSQqkegzQJ4qOTuEIsOw7++8HbUtPb6p/GFvfAVoZST\ng82JB8YhHH1o54teQwd6kOvrKynx3CsfglAm7OfKZm2xj/YEfhnIl5DZWAym2M/S\nCHJYK1gut34Y2Mx//HFqLI+AnfldmAS1APAxjAECAwEAAaM4MDYwDAYDVR0TAQH/\nBAIwADAOBgNVHQ8BAf8EBAMCB4AwFgYDVR0lAQH/BAwwCgYIKwYBBQUHAwIwDQYJ\nKoZIhvcNAQEFBQADggEBAAJQfvTLfEuBl5eI0kfxaIeLcnVEqnXdEILKk9lw2exO\n3iz4lKc4tuVplJi0Xn3jAFLJj1P7MKcVWCHNRV2rsuS1jdSougOCNXdlbCXJd5rF\nKjkbpq8gzzpRyrliw/Fbx+R1hlHymzMh8iJ/UsyXJK0FpbIqnArLf0HZ8/oQhQzG\nKHhK73+LEY8kbClzTg3wlQJU5nv3y6Q+43z1GDGCkihzDEEApEBrlSCOmsEhLo+Z\nWZo3l0AhGPJiAGdA/PPFbjsJEMK9/E7i1aDj9FGd0l9OR2KHqF/GzvgC+bTGg1If\niss1I4r1p9ULwn4KbyjOtcNN7eJgwukeq1TS3uRxCXI=\n-----END CERTIFICATE-----\n",
"d33ef69d719064f1c848b6f6b3ba38cc74efdeb6": "-----BEGIN CERTIFICATE-----\nMIIC/DCCAeSgAwIBAgIIe/wwwATSuT0wDQYJKoZIhvcNAQEFBQAwIDEeMBwGA1UE\nAxMVMTE2MjQxMzg0MTE1ODQwMzk0MzUxMCAXDTIxMDQxNDE0MjkyNFoYDzk5OTkx\nMjMxMjM1OTU5WjAgMR4wHAYDVQQDExUxMTYyNDEzODQxMTU4NDAzOTQzNTEwggEi\nMA0GCSqGSIb3DQEBAQUAA4IBDwAwggEKAoIBAQDSa8EW/vm1IDowVS21/CznsMSS\nYsfu9enxLPt4KJbMrFhmlOGScAOoX3eY0tS2aSIscUnBvTAlG29i+u/g2ccIMK9k\nRhzOos/psURAoptpWJD7I1HZSRJWHrQ+PFPfrGHZPUBxka7psMZyyZ/nZEYMi25p\njDu0nOFmAl1+jo1Q/8INN2bJMcOGTARgWOn6nSwUCO3eFcmEjhTrzqkx7pDNZohv\nZ1TvClkH5aWzp2gA4D9Qt5A2a98Ll3Kenl+E/uPP8I1E72Ph0clcgSdSskMHcmBJ\nXllNN3EC5w6WNV75BM9gAL/5mtDbVGNCQdCQSW+WPLPTN6nx4ddW4jUwekrLAgMB\nAAGjODA2MAwGA1UdEwEB/wQCMAAwDgYDVR0PAQH/BAQDAgeAMBYGA1UdJQEB/wQM\nMAoGCCsGAQUFBwMCMA0GCSqGSIb3DQEBBQUAA4IBAQAbXIT7sx25C/rYflcSKv8T\nj3DYwky76mgkV33c58SAAw+ax+JNxvNQ8WygqN19MzVYqN8RVvKcjG7ILkPfjuQu\nNTo7I1yEx9yxq8IBzlGNEYLMkPj4VTap2AYDuZqMLTUeVJMygI3GhEpeIxt4tMGF\nfPPsh3W6O1Iwegw9yBLpNqgh2C1MtyUAn4rmu+cDTwB/rw7V8pNWnvPvh/bYoV+Q\nWYBIQLtcKxg0RlQcfiVE5QRMBc94WvPl78g14QhXp/7XN36oH6NPj2ztfaRqZ5P7\n6nb2/tNfrXbhXuhW203RFxSruKkYk323KomuNQyyMAfqzOqlT5jYw8e78Edi8/Tq\n-----END CERTIFICATE-----\n",
"b53fb005f06c972bcb3d0656a94783a046dc1c27": "-----BEGIN CERTIFICATE-----\nMIIDTDCCAjSgAwIBAgIIZp+s6sy1pjAwDQYJKoZIhvcNAQEFBQAwSTFHMEUGA1UE\nAww+aWFwLTE4MC5hcGktODUwNzc3OTQ0Njg5ODM3MDY4NS03MzIzODcuaWFtLmdz\nZXJ2aWNlYWNjb3VudC5jb20wHhcNMjQwNjA4MjEwNDE2WhcNMjQwNjI1MDkxOTE2\nWjBJMUcwRQYDVQQDDD5pYXAtMTgwLmFwaS04NTA3Nzc5NDQ2ODk4MzcwNjg1LTcz\nMjM4Ny5pYW0uZ3NlcnZpY2VhY2NvdW50LmNvbTCCASIwDQYJKoZIhvcNAQEBBQAD\nggEPADCCAQoCggEBAMHod06o8A1yxXfk4tv63v1cEEji0ahjCLlKD+D53Nuyiee9\n2rdRm3jrpYs7XYgfhLgHpzT9kVPzghrJANLvhkDLMNcOtCeGmSOcsz7+iXfQX+Kw\n5FnBU66GYvsFWey4ckiPQx3tYlU9T/ekqn+gfVcpv+gmmJIthlv3eZPpoCqUZMot\nEZc1RqNhznGk80jVGApLHxMTrobz1zwOSFh6xztU2mueqVrLSC6TMAvaM60bs1GB\nswHGPQdW5G9IySv/95+dycnbhuZ/CMR5TcIuGN3Ip7QEo8nk0w1kA+8DYOIMzgQD\neh2Zjeu2SjXb9uQz+kQyWLcqcxmGBCasCQQLBI8CAwEAAaM4MDYwDAYDVR0TAQH/\nBAIwADAOBgNVHQ8BAf8EBAMCB4AwFgYDVR0lAQH/BAwwCgYIKwYBBQUHAwIwDQYJ\nKoZIhvcNAQEFBQADggEBAJuLeg9dkqaW/sF8nkj6+rkz3EL5LLjFY/MQfVGOpfAb\nXYsvMSm3pAGOrEbQQOgF99gVZK32KMU16XM/bsDik2U7mb14xAPnUfyB3avilqk0\n0/x4DO2lluvJdajzJEIctDIzMpDzfH5UhrY7CjbUzgCFcIUm8lNSIW/3xdGC8F1+\nptNcE5PtlruybwPwGQUGgHo3wF7l3QxyPv17DjkFiiw1GjNrQAzL/qyuj+N4wpT+\nN6PSkNuJ4FcZ2dSgzsrGdbTZpdjBnJukLFSUHkH6W6Z1Kw93Na8AQCrmujw6Gy0j\nx7lKVe8VHSHfi255mG3t0DtLm1NA+lUk0biE9eKPjoU=\n-----END CERTIFICATE-----\n"
}
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.
Either the x509 url or the client_email work as capture groups.
Description:
This PR refactors the validation logic for GCP service account keys. Instead of directly calling the Google API to verify the credentials using
google.CredentialsFromJSON
, we now perform the validation by comparing the public keys.Key changes:
isValidGCPServiceAccountKey
to better reflect its purpose.ClientX509CertURL
field of thegcpKey
struct.PrivateKey
field of thegcpKey
struct.true
, indicating a valid service account key. Otherwise, it returnsfalse
.Benefits:
Checklist:
make test-community
)?make lint
this requires golangci-lint)?