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

SNI support #5097

Merged
merged 1 commit into from Oct 15, 2015

Conversation

Projects
None yet
3 participants
@liggitt
Contributor

liggitt commented Oct 13, 2015

Enable SNI certificate selection for the master, asset server, and node
Allow setting a certificate to use for a particular hostname or wildcard.
A default cert (servingInfo.{certFile,keyFile}) is required in order to specify per host certs.

servingInfo:
  certFile: master.server.crt
  keyFile: master.server.key
  namedCertificates:
  - certFile: custom.crt
    keyFile: custom.key
    names:
    - "customhost.com"
    - "api.customhost.com"
    - "console.customhost.com"
  - certFile: wildcard.crt
    keyFile: wildcard.key
    names:
    - "*.wildcardhost.com"
  ...

TODO:

Follow-ups:

  • improve config file file reference extraction (maybe using reflection?) #5125
@liggitt

This comment has been minimized.

Show comment
Hide comment
@liggitt
Contributor

liggitt commented Oct 13, 2015

@liggitt liggitt changed the title from WIP - SNI support to SNI support Oct 13, 2015

@liggitt

This comment has been minimized.

Show comment
Hide comment
@liggitt

liggitt Oct 13, 2015

Contributor

[test]

Contributor

liggitt commented Oct 13, 2015

[test]

@liggitt

This comment has been minimized.

Show comment
Hide comment
@liggitt

liggitt Oct 14, 2015

Contributor

@deads2k PTAL

Contributor

liggitt commented Oct 14, 2015

@deads2k PTAL

@@ -120,6 +121,10 @@ func GetMasterFileReferences(config *MasterConfig) []*string {
refs = append(refs, &config.ServingInfo.ServerCert.CertFile)
refs = append(refs, &config.ServingInfo.ServerCert.KeyFile)
refs = append(refs, &config.ServingInfo.ClientCA)
for i := range config.ServingInfo.NamedCertificates {

This comment has been minimized.

@deads2k

deads2k Oct 14, 2015

Contributor

This is causing is problems. Embedding in ServingInfo makes sense, but now you're trying to chase all ServingInfo fields in the config and you've missed some. I agree that it would be crazy to attach this to say EtcdConfig, but it could be there anyway. Maybe a reflective recurse would be better?

@deads2k

deads2k Oct 14, 2015

Contributor

This is causing is problems. Embedding in ServingInfo makes sense, but now you're trying to chase all ServingInfo fields in the config and you've missed some. I agree that it would be crazy to attach this to say EtcdConfig, but it could be there anyway. Maybe a reflective recurse would be better?

This comment has been minimized.

@liggitt

liggitt Oct 14, 2015

Contributor

I only added references for the ones we support (I explicitly disallow it in etcdConfig via validation, since etcd doesn't support SNI)... add to HTTPServingInfo instead? felt weird there

@liggitt

liggitt Oct 14, 2015

Contributor

I only added references for the ones we support (I explicitly disallow it in etcdConfig via validation, since etcd doesn't support SNI)... add to HTTPServingInfo instead? felt weird there

This comment has been minimized.

@deads2k

deads2k Oct 14, 2015

Contributor

I only added references for the ones we support (I explicitly disallow it in etcdConfig via validation, since etcd doesn't support SNI)... add to HTTPServingInfo instead? felt weird there

I'm just looking at this function and decision about file ref or not is based mostly on type. I'd feel better about finding all references to types and adding them as file refs.

@deads2k

deads2k Oct 14, 2015

Contributor

I only added references for the ones we support (I explicitly disallow it in etcdConfig via validation, since etcd doesn't support SNI)... add to HTTPServingInfo instead? felt weird there

I'm just looking at this function and decision about file ref or not is based mostly on type. I'd feel better about finding all references to types and adding them as file refs.

}
namedCerts := map[string]*tls.Certificate{}
for _, namedCertificate := range namedCertificates {
cert, err := tls.LoadX509KeyPair(namedCertificate.CertFile, namedCertificate.KeyFile)

This comment has been minimized.

@deads2k

deads2k Oct 14, 2015

Contributor

Will go validate that the cert is legal to serve for what you're asking? If not, you've already loaded it can you inspect for its validity to serve the requested names here?

@deads2k

deads2k Oct 14, 2015

Contributor

Will go validate that the cert is legal to serve for what you're asking? If not, you've already loaded it can you inspect for its validity to serve the requested names here?

This comment has been minimized.

@liggitt

liggitt Oct 14, 2015

Contributor

no, it'll let you use a cert to serve anything you want (it'll still secure the connection, the client just won't be able to validate the hostname). I can add validation and warn if the cert is invalid or doesn't contain a DNS name matching the configured names

@liggitt

liggitt Oct 14, 2015

Contributor

no, it'll let you use a cert to serve anything you want (it'll still secure the connection, the client just won't be able to validate the hostname). I can add validation and warn if the cert is invalid or doesn't contain a DNS name matching the configured names

This comment has been minimized.

@deads2k

deads2k Oct 14, 2015

Contributor

no, it'll let you use a cert to serve anything you want (it'll still secure the connection, the client just won't be able to validate the hostname). I can add validation and warn if the cert is invalid or doesn't contain a DNS name matching the configured names

Yeah, I see no valid reason for choosing a different cert that won't validate. I would do it here, not in the validate methods, since we haven't been loading certs there.

@deads2k

deads2k Oct 14, 2015

Contributor

no, it'll let you use a cert to serve anything you want (it'll still secure the connection, the client just won't be able to validate the hostname). I can add validation and warn if the cert is invalid or doesn't contain a DNS name matching the configured names

Yeah, I see no valid reason for choosing a different cert that won't validate. I would do it here, not in the validate methods, since we haven't been loading certs there.

This comment has been minimized.

@liggitt

liggitt Oct 14, 2015

Contributor

I do... they might want to use a cert signed by their corporate CA, so signer validation would pass, even if hostname validation did not

@liggitt

liggitt Oct 14, 2015

Contributor

I do... they might want to use a cert signed by their corporate CA, so signer validation would pass, even if hostname validation did not

This comment has been minimized.

@deads2k

deads2k Oct 14, 2015

Contributor

I do... they might want to use a cert signed by their corporate CA, so signer validation would pass, even if hostname validation did not

"Security is hard and users don't know any better"

@deads2k

deads2k Oct 14, 2015

Contributor

I do... they might want to use a cert signed by their corporate CA, so signer validation would pass, even if hostname validation did not

"Security is hard and users don't know any better"

@liggitt

This comment has been minimized.

Show comment
Hide comment
@liggitt

liggitt Oct 14, 2015

Contributor

comments addressed

Contributor

liggitt commented Oct 14, 2015

comments addressed

Show outdated Hide outdated pkg/cmd/util/net.go
@deads2k

This comment has been minimized.

Show comment
Hide comment
@deads2k

deads2k Oct 14, 2015

Contributor

minor comments. lgtm otherwise.

Contributor

deads2k commented Oct 14, 2015

minor comments. lgtm otherwise.

@liggitt

This comment has been minimized.

Show comment
Hide comment
@liggitt

liggitt Oct 14, 2015

Contributor

[merge]

Contributor

liggitt commented Oct 14, 2015

[merge]

@openshift-bot

This comment has been minimized.

Show comment
Hide comment
@openshift-bot

openshift-bot Oct 14, 2015

Member

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/merge_pull_requests_origin/3627/) (Image: devenv-fedora_2467)

Member

openshift-bot commented Oct 14, 2015

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/merge_pull_requests_origin/3627/) (Image: devenv-fedora_2467)

@openshift-bot

This comment has been minimized.

Show comment
Hide comment
@openshift-bot

openshift-bot Oct 14, 2015

Member

continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_requests_origin/5796/)

Member

openshift-bot commented Oct 14, 2015

continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_requests_origin/5796/)

@openshift-bot

This comment has been minimized.

Show comment
Hide comment
@openshift-bot

openshift-bot Oct 14, 2015

Member

Evaluated for origin test up to 81b520f

Member

openshift-bot commented Oct 14, 2015

Evaluated for origin test up to 81b520f

@openshift-bot

This comment has been minimized.

Show comment
Hide comment
@openshift-bot

openshift-bot Oct 15, 2015

Member

Evaluated for origin merge up to 81b520f

Member

openshift-bot commented Oct 15, 2015

Evaluated for origin merge up to 81b520f

openshift-bot added a commit that referenced this pull request Oct 15, 2015

@openshift-bot openshift-bot merged commit 47d1103 into openshift:master Oct 15, 2015

1 of 3 checks passed

continuous-integration/openshift-jenkins/merge Testing
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
continuous-integration/openshift-jenkins/test Tested
Details

@liggitt liggitt deleted the liggitt:multi-cert branch Oct 15, 2015

@liggitt liggitt referenced this pull request Nov 4, 2015

Closed

Document SNI certificates #1143

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment