add a new collector that gets information about a server's TLS certificates #1765
Conversation
|
example collected output: {
"peer_certificates": [
{
"issuer": "localhost",
"subject": "localhost",
"serial": "223988171543336272920354702839529366910",
"not_before": "2025-03-31T22:51:45Z",
"not_after": "2025-03-31T23:51:45Z",
"is_ca": false,
"raw": "MIIC/jCCAeagAwIBAgIRAKiCjXwzGYpbzqguTLBYZX4wDQYJKoZIhvcNAQELBQAwFDESMBAGA1UEAxMJbG9jYWxob3N0MB4XDTI1MDMzMTIyNTE0NVoXDTI1MDMzMTIzNTE0NVowFDESMBAGA1UEAxMJbG9jYWxob3N0MIIBIjANBgkqhkiG9w0BAQEFAAOCAQ8AMIIBCgKCAQEAn2dQnXU+Q/liN7lcG5ik0F5kqkD341IRpohxYsL9yA2w2uxGHxstajYuh9aiuD40RvQ2XmyGXsX8z7pWPjzDiKPO5EkU+0ahPi3kHAGyy4nD/tcevtUR7fOO4mirqvxozZkhLUAYN3zEn374V+WcdjuogAStBpgRFAltfH/9lLGlET0QFP2j+030JxcNpsLgnRSS/RGmiHZ0Ybk+OaMnj4cOQto0Jo2X51hQL6MYuNu1a1SAy2c8OXMuu9gaj7LwdFHVhSiAUFZHZRBW5Eisp2NXVtGmV/C9ybj7sk6MEGsalb3jCSsmPWKSrAcyH0lgR4r6AKwQd+AgNotOZPl6cwIDAQABo0swSTAOBgNVHQ8BAf8EBAMCBaAwEwYDVR0lBAwwCgYIKwYBBQUHAwEwDAYDVR0TAQH/BAIwADAUBgNVHREEDTALgglsb2NhbGhvc3QwDQYJKoZIhvcNAQELBQADggEBABoFF/8mqT9vS2+awapvjTVfmKQ1fcMB0KVyi28UD3rEEzfAxIT2aViIXoS5bb30esdfFzfFAmWss6E0tHBbul2vccAW/kQPx6XqZGpdEKQc0Dr2uB9FRu4Bx13CfSprMgqeoK1/O/VKtcQ8jQIbLSA9/QIFAJ6Oo3vBSMWRo+izi2NZQ/nIrSbk2a4NpDcv6tfJcSOGfS0JIIrKf/EEu2po7HpD8vE1hCzqsWOhC1+yV46tvOsOFRJ6FEusBlH8Kh4WsSld+S/ARsAoaJyc2Ft77D/YrSBYlaFWSO4VGo4rXP6q2nGcGz7/eXR4n1Q7F9P75/xnZrZakcl5K6wlGgc="
}
]
}And the output for replicated.app: |
pkg/collect/host_tls.go
Outdated
| tlsInfo := types.TLSInfo{} | ||
|
|
||
| conf := &tls.Config{ | ||
| InsecureSkipVerify: true, |
There was a problem hiding this comment.
this should be an option in the collector
There was a problem hiding this comment.
Are you sure? IMO this should just be the default - you want to know the cert regardless of whether the host trusts it
There was a problem hiding this comment.
I'm not 100% on the use case for adding this now, but I can see it being useful later if we let users pass in custom CAs. In that scenario, to make sure we're using the right certs, InsecureSkipVerify should be set to false. Otherwise, if we don't handle it on the collector side, users on the analyzer side would have to figure out if the certificate chain was trusted. Not saying we need to fix this here, but we should probably file a story for it if not done here.
There was a problem hiding this comment.
I would agree with adding a "this cert chain is trusted by the host" bit later, but I think the "is this trusted with the provided CA/host CAs" is already covered by the HTTP collector.
The goal of this is to figure out what certs are being used, and I think getting back an error if you didn't set "allow untrusted" is unintuitive
Co-authored-by: Ethan Mosbaugh <[email protected]>
| @@ -60,6 +60,12 @@ type TimeAnalyze struct { | |||
| Outcomes []*Outcome `json:"outcomes" yaml:"outcomes"` | |||
| } | |||
|
|
|||
| type TLSAnalyze struct { | |||
There was a problem hiding this comment.
Should we name this HostTLSAnalyze to keep it consistent with the others?
Also, similar to collector_shared, you'll need to add this to the HostAnalyze struct as well.
There was a problem hiding this comment.
Should we name this HostTLSAnalyze to keep it consistent with the others?
very few of the others have 'host' in the name - just 4 of 25
pkg/collect/host_tls.go
Outdated
| NotAfter: cert.NotAfter.Format(time.RFC3339), | ||
| IsCA: cert.IsCA, | ||
| Raw: cert.Raw, | ||
| tlsInfo.Error = err.Error() |
There was a problem hiding this comment.
I guess I had imagined you would piggyback on this
troubleshoot/pkg/collect/host_network.go
Line 73 in cbb22e2
|
@emosbaugh to prevent hardcoding the expected cert issuer in analyzers, I've added a new bit of collector + analyzer logic:
This way we have to update the replicated.app server with a new route (pretty easy), but the issuer isn't hardcoded anymore |
|
Putting this on hold - it's not clear this is a good way to detect non-default CAs |
Description, Motivation and Context
In order to determine if there is a MITM proxy, it is useful to know what the certificate is returned by an external endpoint.
This allows doing so, and analyzing the result by comparing the certificate's issuer to an expected value.
Checklist
Does this PR introduce a breaking change?