Skip to content
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

Build full chains out of named_certificates that come with CAs #4920

Closed
wants to merge 2 commits into from
Closed

Build full chains out of named_certificates that come with CAs #4920

wants to merge 2 commits into from

Conversation

gregswift
Copy link
Contributor

@gregswift gregswift commented Jul 28, 2017

I've spent that last little bit trying to figure out why our custom CA wasn't always working. Turns out on 2 of our 3 masters it just wasn't behaving properly. @abutcher and @damaestro helped me troubleshoot.

We were validating with

for x in {1..3}; do echo Q | openssl s_client -showcerts -connect master${x}.example.com:8443 -servername openshift.example.com; done

and

for x in {1..3}; do IP=$(host master${x}.example.com | awk '{print $4}'); echo "Testing ${host}: $(curl -s --resolve openshift.example.com:8443:$IP https://openshift.example.com:8443/healthz/ready || echo fail)"; done

We could never find out why master1 was working, but when we put the full chain (cert+int+root) in the certfile, things started working properly.

So after a quick convo on how to just make this happen, without changing people's inventory files, i created this change set.

You can validate this off to the side fairly easy with this attached playbook.

named_certs.yml.txt

@openshift-bot
Copy link

Can one of the admins verify this patch?
I understand the following commands:

  • bot, add author to whitelist
  • bot, test pull request
  • bot, test pull request once

mode: 0600
with_items: "{{ named_certificates | oo_collect('cafile') }}"
Copy link
Contributor Author

@gregswift gregswift Jul 28, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume oo_collect was used in place of the when statements that I implemented. I'm not sure why it would be preferred, but in moving to the assemble module in ansible I needed access to more than just the cafile in the dictionary.

@openshift-bot
Copy link

Can one of the admins verify this patch?
I understand the following commands:

  • bot, add author to whitelist
  • bot, test pull request
  • bot, test pull request once

@abutcher abutcher self-requested a review July 28, 2017 19:13
mode: 0600
with_items: "{{ named_certificates | oo_collect('cafile') }}"
validate: "openssl verify %s"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this validate is too strict and would only work if we already trusted the root? Using something like this guide to roll your own chain would be a valid configuration for a cluster but would require that clients trust the root.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reason i went this route was that ither the CAfile provided is a fully trusted bundle or the system trusts the CA already. Otherwise you arent necessarily working with a working cert chain.

@openshift-bot
Copy link

Can one of the admins verify this patch?
I understand the following commands:

  • bot, add author to whitelist
  • bot, test pull request
  • bot, test pull request once

1 similar comment
@openshift-bot
Copy link

Can one of the admins verify this patch?
I understand the following commands:

  • bot, add author to whitelist
  • bot, test pull request
  • bot, test pull request once

@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 20, 2017
@openshift-ci-robot openshift-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Feb 5, 2018
@openshift-merge-robot openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 5, 2018
@DanyC97
Copy link
Contributor

DanyC97 commented Aug 20, 2018

@abutcher is this a stale/ not needed PR ?

@abutcher
Copy link
Member

@DanyC97 Yeah this one is stale. We can configure a named certificate with an intermediate certificate authority by putting the files together ourselves before passing to openshift-ansible but there are sharp edges.

@DanyC97
Copy link
Contributor

DanyC97 commented Aug 20, 2018

@abutcher thank you for response. By any chance you have the steps of how you guys created the test certs ?

Reason i'm asking because i spent time to create ssl certs for a private/ internal domain using openssl and sadly web console doesn't use it. Now i cannot use let's encrypt or anything like this since is not a real domain, so at this point i'm not sure if is a prob with openshift or the certs i created.

@abutcher
Copy link
Member

@DanyC97 I created my certs using this guide the last time I tested intermediate CA. If you use that guide make sure that the CAs don't have passphrases (omit -aes256 when creating the key). Is that what you were looking for or something else? I could put steps together but would take some time.

@DanyC97
Copy link
Contributor

DanyC97 commented Aug 21, 2018

@abutcher came across that guide and this time i followed it however i still miss one piece of the puzzle ...

so the steps i've done were:

  1. followed the above guide, generated the server certificates
  2. copied openshift-master.private.dani.com.cert.pem & openshift-master.private.dani.com.key.pem to each master under /etc/origin/master
  3. manually set the master-config.yaml as below and restarted the origin-master-api.service & origin-master-controller.service
assetConfig:
  extensionScripts:
  - /etc/origin/master/openshift-ansible-catalog-console.js
  logoutURL: ''
  masterPublicURL: https://openshift-master.private.dani.com:8443
  publicURL: https://openshift-master.private.dani.com:8443/console/
  servingInfo:
    bindAddress: 0.0.0.0:8443
    bindNetwork: tcp4
    certFile: master.server.crt
    clientCA: ''
    keyFile: master.server.key
    maxRequestsInFlight: 0
    requestTimeoutSeconds: 0
    namedCertificates:
      - certFile: openshift-master.private.dani.com.cert.pem
        keyFile: openshift-master.private.dani.com.key.pem
        names:
          - "openshift-master.private.dani.com"


however accessing the web console is still retrieving the default self-signed master certificate. Even when doing openssl s_client -showcerts -connect openshift-master.private.dani.com:8443 -servername openshift-master.private.dani.com i get same result.

any thoughts ?

P.S - note i'm on 3.7.2 origin ...
P.S1 - to be clear i'm only interested in get rid of the annoying browser message complaining about the certs, if possible i would prefer not to replace the self-signed master certs

@DanyC97
Copy link
Contributor

DanyC97 commented Aug 21, 2018

what is not clear @abutcher is whether the chain & certificate file is required or the key & cert file are okay ?

If you look here you can see it does use the cafile but that key is available only for openshifft router if i'm not wrong ...

@DanyC97
Copy link
Contributor

DanyC97 commented Aug 21, 2018

i found out from @vrutkovs that i need to run the playbooks/byo/openshift-cluster/redeploy-certificates.yml and with openshift_master_overwrite_named_certificates=true set but for an existing cluster that is a very high risk.

And openshift_master_overwrite_named_certificates=false it seems it won't work - ie no change from client/ browser side as i mentioned previously.

@abutcher
Copy link
Member

@DanyC97 Ah, I wasn't sure what the goal was but replacing all certificates should not be necessary to replace or add a new named certificate as long as the internal and external (public) cluster hostnames are different. Do you have an issue where you're discussing this? I think we've moved away from the PR discussion.

@DanyC97
Copy link
Contributor

DanyC97 commented Aug 21, 2018

@abutcher @vrutkovs please close this PR, i'll open an issue to carry on with the discussion.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants