Add More TLS Tests and Verification of TLS Root Certificate #11300
Conversation
…ers from single file:
| const maxVersionError = "'tls_max_version' value %q not supported, please specify one of [tls10,tls11,tls12,tls13]" | ||
|
|
||
| func ListenerChecks(listeners []listenerutil.Listener) error { | ||
| for _, listener := range listeners { |
There was a problem hiding this comment.
Future functionality: check that client related certificate paths exist.
There was a problem hiding this comment.
This should incorporate reading and loading.
| if err != nil { | ||
| return err | ||
| } | ||
| x509Cert, err := x509.ParseCertificate(cert.Certificate[0]) |
There was a problem hiding this comment.
This can be removed.
| } | ||
|
|
||
| // TLSFileChecks contains manual error checks against the TLS configuration | ||
| func TLSFileChecks(certFilePath, keyFilePath string) error { |
There was a problem hiding this comment.
Modularize this to load the cert from a file path separately, and have the checks in a separate function so that we can load from urls as well.
|
|
||
| var err error | ||
| // Perform checks on the TLS Cryptographic Information. | ||
| if err = TLSFileChecks(l.TLSCertFile, l.TLSKeyFile); err != nil { |
There was a problem hiding this comment.
is there a reason to not declare err here with :=?
| } | ||
| err := ListenerChecks(listeners) | ||
| if err == nil { | ||
| t.Error("TLS Config check on fake certificate should fail") |
There was a problem hiding this comment.
i think these should be fails (not errors), since you can't do the next check if err is nil (presumably that's a caught panic with the test infra)
There was a problem hiding this comment.
there are multiple cases like this
There was a problem hiding this comment.
I'm so sorry! I didn't realized you had left some comments @swayne275 with the approval. Yeah, fails here would be better. I'll push a PR out today cleaning that up.
I can also use := for the error var in the cleanup PR. For the !string.Contains() comment below, I can't do that for this particular test, because the error message thrown by the library is a concatenation of a few error messages. I didn't want to copy that whole string here, so I just checked the necessary part to distinguish this failure from the other cases.
Thanks for the comments, and sorry about the premature merge! I'll have followup PR with the improvements ASAP!
There was a problem hiding this comment.
No problem! I wouldn't have approved it if I wasn't okay with it going without fixes/responses to the comments
The panic in tests isn't that big of a deal, it's just a lot cleaner to avoid it
There was a problem hiding this comment.
I'm changing these to Fatals instead of Fails -- Error is equivalent to a Log followed by a Fail, and Fail continues execution. Fatal stops the test with a FailNow.
There was a problem hiding this comment.
oh yeah fatal is probably what i meant, cool!
| if err == nil { | ||
| t.Error("TLS Config check on fake certificate should fail") | ||
| } | ||
| if err.Error() != "tls: private key type does not match public key type" { |
There was a problem hiding this comment.
did you want these to be !string.Contains() as you've done for your other tests?
* master: (78 commits) docs: update vault-helm to 0.11.0 (#11355) Add documentation for vault-csi-provider namespace config (#11344) docs: update vault-k8s to 0.10.0 (#11354) patch(docs): fix link color (#11352) Add TFE/TFC auth plugin to plugin portal (#11348) fix a couple typos (#11343) TLS Diagnose Formatting Fixes (#11342) Add More TLS Tests and Verification of TLS Root Certificate (#11300) Add HA only autopilot to changelog (#11339) Support autopilot when raft is for HA only (#11260) Fixes for db connection file type field (#11331) Fix flakey TestAgent_Template_Retry test (#11332) Darwin/ARM64 build target (#11321) Fix broken OIDC Providers link (#11327) Bug: DB secret engine not showing "Select one" in role select options (#11294) bumping alpine version, improving security (#11271) Run a more strict formatter over the code (#11312) docs: add persistent cache (#11272) Fix a few static analysis findings (#11307) Changing from "changelog" to "release-note" (#11303) ...
This PR expands upon the use of the Verify method to make sure that the root CA, when specified in the listener config, is checked by the diagnose command.
The PR also implements some additional tests verifying prior functionality, and adds a check in for the TLS version configuration in the listener.
Related tickets:
https://hashicorp.atlassian.net/browse/VAULT-1998
https://hashicorp.atlassian.net/browse/VAULT-2002