-
Notifications
You must be signed in to change notification settings - Fork 4.8k
WIP: in depth tls certificate validations for routes #1824
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,13 +1,16 @@ | ||
| package validation | ||
|
|
||
| import ( | ||
| ctls "crypto/tls" | ||
| "crypto/x509" | ||
| "encoding/pem" | ||
| "fmt" | ||
| "strings" | ||
|
|
||
| kval "github.com/GoogleCloudPlatform/kubernetes/pkg/api/validation" | ||
| "github.com/GoogleCloudPlatform/kubernetes/pkg/util" | ||
| "github.com/GoogleCloudPlatform/kubernetes/pkg/util/fielderrors" | ||
|
|
||
| "fmt" | ||
| routeapi "github.com/openshift/origin/pkg/route/api" | ||
| ) | ||
|
|
||
|
|
@@ -68,6 +71,9 @@ func validateTLS(tls *routeapi.TLSConfig) fielderrors.ValidationErrorList { | |
| if len(tls.DestinationCACertificate) == 0 { | ||
| result = append(result, fielderrors.NewFieldRequired("destinationCACertificate")) | ||
| } | ||
|
|
||
| result = append(result, validateTLSCertificates(tls)...) | ||
|
|
||
| //passthrough term should not specify any cert | ||
| case routeapi.TLSTerminationPassthrough: | ||
| if len(tls.Certificate) > 0 { | ||
|
|
@@ -102,9 +108,126 @@ func validateTLS(tls *routeapi.TLSConfig) fielderrors.ValidationErrorList { | |
| if len(tls.DestinationCACertificate) > 0 { | ||
| result = append(result, fielderrors.NewFieldInvalid("destinationCACertificate", tls.DestinationCACertificate, "edge termination does not support destination certificates")) | ||
| } | ||
|
|
||
| result = append(result, validateTLSCertificates(tls)...) | ||
|
|
||
| default: | ||
| msg := fmt.Sprintf("invalid value for termination, acceptable values are %s, %s, %s, or emtpy (no tls specified)", routeapi.TLSTerminationEdge, routeapi.TLSTerminationPassthrough, routeapi.TLSTerminationReencrypt) | ||
| result = append(result, fielderrors.NewFieldInvalid("termination", tls.Termination, msg)) | ||
| } | ||
| return result | ||
| } | ||
|
|
||
| // validateTLSCertificates checks that the certificates passed are able to be parsed. This includes ensuring that | ||
| // the cert/key pair are matching and, if a ca cert is provided it will check the first ca cert against the leaf | ||
| // to ensure that the leaf was signed by the ca | ||
| func validateTLSCertificates(tls *routeapi.TLSConfig) fielderrors.ValidationErrorList { | ||
| if tls == nil { | ||
| return nil | ||
| } | ||
| //edge and reencrypt (the use cases that call this require all 3 (at least) so we can ignore anything | ||
| //that doesn't have all 3 set. It will already fail validation elsewhere | ||
| if len(tls.Certificate) == 0 || len(tls.Key) == 0 || len(tls.CACertificate) == 0 { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. is it ever valid for them to leave the CA blank if it is one that should be included in system roots? For example, if I got my cert from verisign, I don't normally need to provide a CA. Same question applies to the places in validateTLS where CA and DestinationCA are required
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes it is valid, we can relax this requirement. The destination ca should still be required. HAProxy does a verify check with a specific file to the pod so we need to write it and know the location. |
||
| return nil | ||
| } | ||
|
|
||
| result := fielderrors.ValidationErrorList{} | ||
|
|
||
| //check cert/key pair | ||
| checkSignature := true | ||
| cert, err := ctls.X509KeyPair([]byte(decodeNewlines(tls.Certificate)), []byte(decodeNewlines(tls.Key))) | ||
| if err != nil { | ||
| msg := fmt.Sprintf("the certificate and key were not able to be parsed: %s", err.Error()) | ||
| //no value set on this, it is a multi-field validation | ||
| result = append(result, fielderrors.NewFieldInvalid("certificate/key", "", msg)) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. json path - "certificate" or "key", not / |
||
| //since we didn't parse the cert correctly we should skip the signature check in the ca validations | ||
| checkSignature = false | ||
| } | ||
| //cert was parsed, check that the ca can be parsed and has signed the cert if given | ||
| result = append(result, validateCACertBlock(decodeNewlines(tls.CACertificate), "caCertificate", checkSignature, &cert)...) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. if you got an error above,
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. X509KeyPair returns a struct though so it can't be nil and the checkSignature will be set to false so the subsequent validation won't try to mess with it. I can always make it more explicit if it helps readability and set the cert to nil
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. or actually, do the check in an else..that's probably better
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. returning early is more traditional... |
||
| //check that the dest cert can be parsed if given | ||
| result = append(result, validateCACertBlock(decodeNewlines(tls.DestinationCACertificate), "destinationCACertificate", false, &cert)...) | ||
| return result | ||
| } | ||
|
|
||
| // validateCACertBlock iterates through all certs in a ca cert block to ensure that they can be parsed. Optionally, | ||
| // it checks that a ca cert has signed the passed in cert. This checkSignature is used by the caCertificate check but not the | ||
| // destinationCertificate check | ||
| func validateCACertBlock(caCert, field string, checkSignature bool, cert *ctls.Certificate) fielderrors.ValidationErrorList { | ||
| result := fielderrors.ValidationErrorList{} | ||
| if len(caCert) == 0 { | ||
| return result | ||
| } | ||
|
|
||
| //decode and parse the ca cert. When a pem block is decoded the following occurs: | ||
| //1. if a block is parsed it is put into block | ||
| //2. if a block is parsed then the remaining data will be in rest | ||
| //3. if the data is not PEM data then block will be nil and the data will be in rest | ||
| block, rest := pem.Decode([]byte(caCert)) | ||
| var parsedCACert *x509.Certificate | ||
| var err error | ||
|
|
||
| //not pem data | ||
| if block == nil { | ||
| result = append(result, fielderrors.NewFieldInvalid(field, caCert, "unable to parse certificate")) | ||
| } else { | ||
| //pem data found | ||
| //keep the first ca cert so we can check signatures if requested | ||
| parsedCACert, err = x509.ParseCertificate(block.Bytes) | ||
| //bad CA, don't keep parsing the chain and just return | ||
| if err != nil { | ||
| msg := fmt.Sprintf("the %s certificate was not able to be parsed: %s", field, err.Error()) | ||
| result = append(result, fielderrors.NewFieldInvalid(field, caCert, msg)) | ||
| return result | ||
| } | ||
|
|
||
| //check remaining rest data for more pem blocks (a cert chain concatenated in the field) | ||
| if len(rest) > 0 { | ||
| for { | ||
| block, rest = pem.Decode(rest) | ||
| //not pem data | ||
| if block == nil { | ||
| result = append(result, fielderrors.NewFieldInvalid(field, caCert, "unable to parse certificate chain")) | ||
| break | ||
| } | ||
| //check the cert | ||
| _, err := x509.ParseCertificate(block.Bytes) | ||
| if err != nil { | ||
| msg := fmt.Sprintf("the %s certificate chain was not able to be parsed: %s", field, err.Error()) | ||
| result = append(result, fielderrors.NewFieldInvalid(field, caCert, msg)) | ||
| } | ||
| //check if we're done with the pem blocks now | ||
| if len(rest) == 0 { | ||
| break | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| if checkSignature && parsedCACert != nil { | ||
| //load up the leaf cert as an x509 certificate so we can check the signatures | ||
| //this shouldn't die since we were able to parse it in validateTLSCertificates and tls.X509KeyPair performs this same | ||
| //check, it just doesn't set the .Leaf value. | ||
| leaf, err := x509.ParseCertificate(cert.Certificate[0]) | ||
| if err != nil { | ||
| msg := fmt.Sprintf("the certificate was not able to be parsed %s", err.Error()) | ||
| //no value set on this because at this time we have a decoded pem block, not what was originally submitted | ||
| result = append(result, fielderrors.NewFieldInvalid("certificate", "", msg)) | ||
| return result | ||
| } | ||
|
|
||
| err = leaf.CheckSignatureFrom(parsedCACert) | ||
| if err != nil { | ||
| msg := fmt.Sprintf("invalid ca cert: %s", err.Error()) | ||
| //no value set because it is a multi-field validation | ||
| result = append(result, fielderrors.NewFieldInvalid("certificate/caCertificate", "", msg)) | ||
| } | ||
| } | ||
|
|
||
| return result | ||
| } | ||
|
|
||
| // decodeNewlines is utility to remove the json formatted newlines from a cert | ||
| func decodeNewlines(s string) string { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. the fact that we need to do this makes me feel like we're missing something... newline to []byte and back should be transparent... we shouldn't need to fiddle with escaped newlines
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this is a holdover from defining a cert inside the route's json. The newlines need escaped in the json and translate just fine when you write the file to disk. When you try to load the json value like I'm doing though...not so much. If there is a better way then we should definitely use it and get rid of this method.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. newlines should be in the JSON strings as "\n"... are they not? if so, parsing the JSON should result in a string with newlines, not literal "\n" strings |
||
| return strings.Replace(s, "\\n", "\n", -1) | ||
| } | ||
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.
s/emtpy/empty/