Add identity-based TLS auth and UI for client/daemon auth#8265
Add identity-based TLS auth and UI for client/daemon auth#8265dmcgowan wants to merge 20 commits intomoby:masterfrom
Conversation
804f4dd to
4862762
Compare
docker/docker.go
Outdated
There was a problem hiding this comment.
@vieux seems the flag is not used, just the variable. This could definitely be cleaned up.
4862762 to
d14a039
Compare
|
Updated authorized keys and allowed hosts flags according to @bfirsh recommendation |
|
@dmcgowan error in tests: |
c448912 to
1ab43b0
Compare
|
@vieux the integration tests have been fixed |
api/server/server.go
Outdated
There was a problem hiding this comment.
I think we should just print the warning everytime you use insecure
There was a problem hiding this comment.
Hmm, I agree we are trying to steer users away from using non-TLS on any tcp connection. Let me think more about how the wording will need to change. Should definitely be switched back to log.Infof, not sure why it changed to Debugf.
There was a problem hiding this comment.
maybe "DON'T BIND INSECURELY ON A TCP ADDRESS IF YOU DON'T KNOW WHAT YOU'RE DOING" and the check is just for "Insecure"
1ab43b0 to
105997c
Compare
api/server/server.go
Outdated
There was a problem hiding this comment.
is there a reason this isn't just named err
There was a problem hiding this comment.
not likely a good one, just didn't get changed when refactored, I can updated
|
@dmcgowan Needs rebase + nits correction |
105997c to
2bfeade
Compare
|
Rebased and fixed changes. Another commit was added to put back what was removed from #8563. The reason for the removal (accidental key file root ownership) has still not been address though and likely should be address before merging this. |
|
seems to need docs to go with it. |
|
@dmcgowan This is a pretty major change to how Docker works, but this pull request doesn't make it clear what it does. Is there a document somewhere with an overview of how this system works, both from a UI and technical perspective? Perhaps we could link to that or include it directly in the pull request description here. I think changing the title might help people understand the implications of this change too. "Add UI for client/daemon auth and enable TLS by default" maybe. |
Signed-off-by: Derek McGowan <[email protected]> (github: dmcgowan)
Signed-off-by: Derek McGowan <[email protected]> (github: dmcgowan)
Signed-off-by: Ben Firshman <[email protected]>
- Removed integration tests which are covered by new tests. - Disabled tests on Drone for same reason as 07cedaa Signed-off-by: Ben Firshman <[email protected]>
Signed-off-by: Ben Firshman <[email protected]>
- Can switch between "identity", "cert" and "none"
- Replaced --insecure with --auth=none
- Moved logic to generate TLS configs to separate auth files
- Renamed --tlscert -> --auth-cert
- Renamed --tlskey -> --auth-key
- Renamed --tlscacert -> --auth-ca
- Add DOCKER_AUTH_{CERT,KEY,CA} envvars for configuring cert auth.
- Added backwards compatibility for --tls... options. The existing tests
for this have been left to ensure correct behaviour.
The DOCKER_CERT_PATH environment variable has been disabled for these
new options. Changing behaviour on whether files exist in the default
location (~/.docker/cert.pem, etc) seems dangerous if these files
are accidentally missing. This option was added in the first place to
make boot2docker work with cert auth, but now we have identity auth,
this is unnecessary.
Signed-off-by: Ben Firshman <[email protected]>
- Change allowed-hosts to known-hosts since known hosts is a more familiar concept than allowed hosts for similar functionality - Add deprecation warning to old tls flags - Use dash for key files instead of underscore for consistency with other key files Signed-off-by: Derek McGowan <[email protected]> (github: dmcgowan)
Signed-off-by: Derek McGowan <[email protected]> (github: dmcgowan)
Signed-off-by: Derek McGowan <[email protected]> (github: dmcgowan)
Signed-off-by: Ben Firshman <[email protected]>
Signed-off-by: Derek McGowan <[email protected]> (github: dmcgowan)
Signed-off-by: Derek McGowan <[email protected]> (github: dmcgowan)
Signed-off-by: Ben Firshman <[email protected]>
Signed-off-by: Ben Firshman <[email protected]>
Signed-off-by: Ben Firshman <[email protected]>
Docker-DCO-1.1-Signed-off-by: Sven Dowideit <[email protected]> (github: SvenDowideit)
the code is using path.Split, path.Join etc. instead of filepath.Split, filepath.Join. path assumes '/' separators, but Windows of course uses '\'. Talk about the gift that keeps on giving...I've been fixing bugs like this in portable code since 1983. See docker-archive-public/docker.machine#76 Signed-off-by: John Gossman <[email protected]>
Signed-off-by: Derek McGowan <[email protected]>
Update the flag processor to use the HOME directory for client configuration file defaults and the /etc directory for daemon configuration file defaults Signed-off-by: Derek McGowan <[email protected]> (github: dmcgowan)
Signed-off-by: Derek McGowan <[email protected]> (github: dmcgowan)
e83855a to
038916d
Compare
|
Considering the amount of review required, and the extra eyes that security related PR mandates, I'm removing the 1.5 milestone (freezes on Jan 20th). |
There was a problem hiding this comment.
This else is unnecessary and reduces readability
|
@bfirsh can you please get us some information on the reasons why on this approach? We can continue this discussion on the original proposal. Please reopen when it is ready and that auth code is reviewed by the qualified parties. |
Add identity-based TLS authentication and use public keys to authenticate access.
This is an implementation of the proposal from #7667.
FSNotify support has not been added to detect changes to authorized keys but may be added in a follow up PR.