cli: prompt to accept the server TLS certificate for manual verification #2177
Conversation
dnephin
force-pushed
the
dnephin/cli-accept-cert
branch
from
8726982 to
446f6aa
Compare
|
|
dnephin
force-pushed
the
dnephin/cli-accept-cert
branch
from
446f6aa to
f0abc35
Compare
dnephin
force-pushed
the
dnephin/cli-accept-cert
branch
6 times, most recently
from
553d73f to
08a6424
Compare
dnephin
requested review from
BruceMacD,
kimskimchi,
jmorganca and
pdevine
as code owners
dnephin
force-pushed
the
dnephin/cli-accept-cert
branch
2 times, most recently
from
35a0bf3 to
bde40b7
Compare
|
@BruceMacD this should be working on MacOS now. We have to repeat the request without the system cert pool to get the right error type. |
| destinations, err := client.ListDestinations(api.ListDestinationsRequest{}) | ||
| if err != nil { | ||
| return nil | ||
| return err |
There was a problem hiding this comment.
Thanks for catching this
|
|
||
| Certificate | ||
|
|
||
| Subject: %[1]s |
There was a problem hiding this comment.
This is awesome. I wonder if we can trim this down a bit – e.g. do we need validity period, SANs, etc?
There was a problem hiding this comment.
Ok, I removed the SAN fields. We definitely don't need those since they will be verified automatically by the client.
I left the validity times, but I added a human readable version first which hopefully makes it read better.
dnephin
force-pushed
the
dnephin/cli-accept-cert
branch
from
bde40b7 to
84c21c1
Compare
internal/cmd/login.go
Outdated
| DNS Names: %[5]s | ||
| IP Addresses: %[6]v | ||
|
|
||
| SHA-256 Fingerprint |
There was a problem hiding this comment.
Does this get printed in the server logs?
There was a problem hiding this comment.
It does, but unfortunately only when the cert is first created.
As part of #2176 I'd like to change cert generation to happen at startup, instead of on first request. If we make that change we can have the fingerprint printed on every startup, instead of only when it is first created. That will ensure that the value is always in the logs for the pod. I believe right now it'll be hard to find if the server pod was recreated after the cert is generated.
The admin can also generate the the value using:
openssl x509 -noout -fingerprint -sha256 -inform pem -in
As long as they know where to find the certificate.
There was a problem hiding this comment.
Makes sense, we can address that in a follow up change that moves cert generation to server start
to allow passing a TlsConfig with a cert pool.
And resolve a few TODOs around errors
So that as we upgrade the version of this config, tests should continue to use the latest version Also fixes a TODO about a failing test. The config migration was doing something surprising when the config had no version.
By using the new Description field. Also fix a bug, previously the cerificate was not being saved as PEM encoded on the second save.
previously our tests were not catching this problem because of the return nil. With that fixed, the tests fail properly.
On darwin the error was of the wrong type when using the system cert pool
dnephin
force-pushed
the
dnephin/cli-accept-cert
branch
from
ab498d9 to
6071725
Compare
Summary
This PR adds a new prompt to
infra loginwhen the servers TLS certificate is not already trusted. The prompt gives the user information about the server certificate and allows them to manually trust it.Once trusted, the user will be able to login again to the same server without being prompted again.
This is a significant enhancement over
--skip-tls-verifybecause even if the user doesn't properly inspect the fingerprint, the only opportunity for a man-in-the-middle attack is on that first request. Every subsequent request is secure.In a follow up PR I will:
--skip-tls-verifyRelated Issues
Resolves #1541