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

Reduce certificate expiry time #970

Merged
merged 10 commits into from
Mar 10, 2020
Merged

Reduce certificate expiry time #970

merged 10 commits into from
Mar 10, 2020

Conversation

joedborg
Copy link
Contributor

@joedborg joedborg commented Feb 18, 2020

Trying to address #945

@joedborg joedborg added the kind/bug Something isn't working label Feb 18, 2020
@joedborg joedborg self-assigned this Feb 18, 2020
@joedborg joedborg marked this pull request as ready for review February 19, 2020 21:46
@joedborg
Copy link
Contributor Author

This is still blocked by both Chrome and Safari (although I believe this is a bug in Safari as it lets you add the exception and later acknowledges this), but means that the dates on certificates are now valid.

local IP_ADDRESSES="$(get_ips)"

cp ${SNAP_DATA}/certs/ca.conf.template ${SNAP_DATA}/certs/ca.conf.rendered
if ! [ "$IP_ADDRESSES" == "127.0.0.1" ] && ! [ "$IP_ADDRESSES" == "none" ]
Copy link
Member

Choose a reason for hiding this comment

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

This part does not look right.

The ca.conf.template does not have a MOREIPS line. I think we do not need this part.

fi

if ! "${SNAP}/usr/bin/cmp" -s "${SNAP_DATA}/certs/ca.conf.rendered" "${SNAP_DATA}/certs/ca.conf"; then
cp ${SNAP_DATA}/certs/ca.conf.rendered ${SNAP_DATA}/certs/ca.conf
Copy link
Member

Choose a reason for hiding this comment

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

This does not look right. I think that if there is a change in the ca we should set the force flag on this function so all related certificates are recreated.

# Generate apiserver CA
if ! [ -f ${SNAP_DATA}/certs/ca.crt ]; then
${SNAP}/usr/bin/openssl req -x509 -new -nodes -key ${SNAP_DATA}/certs/ca.key -subj "/CN=10.152.183.1" -days 10000 -out ${SNAP_DATA}/certs/ca.crt
${SNAP}/usr/bin/openssl req -x509 -new -sha256 -nodes -key ${SNAP_DATA}/certs/ca.key -subj "/CN=10.152.183.1" -out ${SNAP_DATA}/certs/ca.crt -config ${SNAP_DATA}/certs/ca.conf
Copy link
Member

Choose a reason for hiding this comment

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

This creates the certificate only the first time. Should we follow the pattern we have for the rest of the certificates where is someone changes the template we trigger the re-issue of the certs?

@@ -167,18 +167,14 @@ spec:
spec:
containers:
- name: kubernetes-dashboard
image: kubernetesui/dashboard:v2.0.0-beta5
image: kubernetesui/dashboard:v2.0.0-rc5
Copy link
Member

Choose a reason for hiding this comment

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

We may want to have this as a separete PR so it does not get blocked by the rest of the work we are doing here.

@balchua
Copy link
Collaborator

balchua commented Mar 5, 2020

Safari is reducing the certificate validity to 1 year. Im not a mac user, but just wonderin if that will have an impact on this PR.

@ktsakalozos
Copy link
Member

Safari is reducing the certificate validity to 1 year

@balchua, @joedborg, I see bot of you agree on a 365 certificate validity so lets go with that (reverted the last commit).

Copy link
Member

@ktsakalozos ktsakalozos left a comment

Choose a reason for hiding this comment

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

LGTM +1 thank you @joedborg

@ktsakalozos ktsakalozos merged commit f77eb17 into master Mar 10, 2020
@ktsakalozos ktsakalozos deleted the bug/cert-expiry branch April 4, 2020 20:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants