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

Replace secrets, node acceptance, and CA hash with join tokens #1189

Merged
merged 1 commit into from Jul 20, 2016

Conversation

Projects
None yet
7 participants
@aaronlehmann
Copy link
Collaborator

aaronlehmann commented Jul 19, 2016

Implement the swarmkit parts of the flow described in moby/moby#24430.

I need to add a few tests, but otherwise this PR is nearly ready.

cc @diogomonica @aluzzardi @stevvooe @tonistiigi @tiborvass

@aaronlehmann aaronlehmann force-pushed the aaronlehmann:join-tokens branch from b31be71 to eba0481 Jul 19, 2016

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Jul 19, 2016

Current coverage is 55.35%

Merging #1189 into master will increase coverage by 0.45%

@@             master      #1189   diff @@
==========================================
  Files            77         77          
  Lines         12193      12177    -16   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits           6694       6741    +47   
+ Misses         4579       4533    -46   
+ Partials        920        903    -17   

Sunburst

Powered by Codecov. Last updated by eacedad...dd9a504

@aaronlehmann aaronlehmann force-pushed the aaronlehmann:join-tokens branch from eba0481 to 26dda2a Jul 19, 2016

@aaronlehmann

This comment has been minimized.

Copy link
Collaborator

aaronlehmann commented Jul 19, 2016

Added a controlapi test to cover token rotation. Still need to add a test in the ca package that covers rotation.

bytes csr = 2 [(gogoproto.customname) = "CSR"];

// Secret represents a user-provided string that is necessary for new
// nodes to join the cluster
string secret = 3;

This comment has been minimized.

@stevvooe

stevvooe Jul 19, 2016

Contributor

Does this become token?

@aaronlehmann aaronlehmann force-pushed the aaronlehmann:join-tokens branch from 26dda2a to 424ae33 Jul 19, 2016

@aaronlehmann aaronlehmann changed the title [WIP] Replace secrets, node acceptance, and CA hash with join tokens Replace secrets, node acceptance, and CA hash with join tokens Jul 19, 2016

@aaronlehmann

This comment has been minimized.

Copy link
Collaborator

aaronlehmann commented Jul 19, 2016

Added a test in the ca package for token rotation. Removed WIP.

PTAL

@aaronlehmann aaronlehmann force-pushed the aaronlehmann:join-tokens branch from 424ae33 to 3065ede Jul 19, 2016

@dperny

This comment has been minimized.

Copy link
Member

dperny commented Jul 19, 2016

This will also require fixing the integration tests in docker/docker, right? Is this an API-breaking change?

@aaronlehmann

This comment has been minimized.

Copy link
Collaborator

aaronlehmann commented Jul 19, 2016

Don't merge this until the docker/docker PR is ready.

}

secret, err := cmd.Flags().GetString("secret")
joinToken, err := cmd.Flags().GetString("secret")

This comment has been minimized.

@tonistiigi

tonistiigi Jul 19, 2016

Member

s/secret/join-token/

This comment has been minimized.

@diogomonica

diogomonica Jul 19, 2016

Contributor

not secret anymore.

@aaronlehmann aaronlehmann force-pushed the aaronlehmann:join-tokens branch 3 times, most recently from 4f8aca7 to 5d1058d Jul 19, 2016


// Secret to be used on the first certificate request.
Secret string
// JoinToken is the token to be used on the first certificate request.

This comment has been minimized.

@diogomonica

diogomonica Jul 19, 2016

Contributor

Each node's first certificate request.

Maybe also add that the default implementation also includes the CA Hash?

@@ -34,11 +34,15 @@ message NodeCertificateStatusResponse {
}

message IssueNodeCertificateRequest {
NodeRole role = 1;
// DEPRECATED: Role is now selected based on which secret is matched.
NodeRole role = 1 [deprecated=true];

This comment has been minimized.

@diogomonica

diogomonica Jul 19, 2016

Contributor

Why are we deprecating VS removing? Are we already on API freeze mode?

This comment has been minimized.

@aaronlehmann

aaronlehmann Jul 19, 2016

Collaborator

I think we could remove this one.

For the stuff that persists in raft, I'd prefer to avoid messing up compatiblity, because it will mean anyone upgrading from a previous -rc will have a broken cluster with a bunch of confusing protobuf errors.

This comment has been minimized.

@dperny

dperny Jul 19, 2016

Member

Isn't it a thing with protobuf that you can drop number fields as long as you don't reuse numbers because when the newer version unmarshals older protobufs, it'll just ignore those fields? Does that not apply in this case?

This comment has been minimized.

@aaronlehmann

aaronlehmann Jul 19, 2016

Collaborator

That's correct. The fields are left in as deprecated to avoid accidentally reusing the field numbers.

This comment has been minimized.

@dperny

dperny Jul 19, 2016

Member

Could also do, like, reserved 1; // formerly NodeRole, now deprecated, but I don't know if that would be an improvement.


var nn, digest big.Int
nn.SetBytes(secretBytes[:])
digest.SetString(rootCA.Digest.Hex(), 16)

This comment has been minimized.

@diogomonica

diogomonica Jul 19, 2016

Contributor

Isn't this giving a token that has the CA hash as base16 instead of base36?

This comment has been minimized.

@aaronlehmann

aaronlehmann Jul 19, 2016

Collaborator

No, it is parsing the hex representation of the hash to turn it into base36.

var nn, digest big.Int
nn.SetBytes(secretBytes[:])
digest.SetString(rootCA.Digest.Hex(), 16)
return fmt.Sprintf("SWMTKN-1-%s-%0[2]*s", digest.Text(joinTokenBase), maxGeneratedSecretLength, nn.Text(joinTokenBase))

This comment has been minimized.

@diogomonica

diogomonica Jul 19, 2016

Contributor

Why are we using maxGeneratedSecretLength instead of the actual len?

This comment has been minimized.

@aaronlehmann

aaronlehmann Jul 19, 2016

Collaborator

We're zero-padding the secret to 128 bits. It's not really necessary, but I felt like this would be a good idea so the length of the secret doesn't leak the upper bits. Happy to change it.

This comment has been minimized.

@tonistiigi

tonistiigi Jul 19, 2016

Member

Why pad only secret and not digest?

This comment has been minimized.

@aaronlehmann

aaronlehmann Jul 19, 2016

Collaborator

I'll pad the digest as well.

return fmt.Sprintf("SWMTKN-1-%s-%0[2]*s", digest.Text(joinTokenBase), maxGeneratedSecretLength, nn.Text(joinTokenBase))
}

func tokenCAHash(token string) (digest.Digest, error) {

This comment has been minimized.

@diogomonica

diogomonica Jul 19, 2016

Contributor

getCAHashFromToken?

@@ -402,6 +340,13 @@ func (s *Server) Run(ctx context.Context) error {
state.EventUpdateNode{},
state.EventUpdateCluster{},
)

// Do this after updateCluster has been called, so isRunning never

This comment has been minimized.

@diogomonica

diogomonica Jul 19, 2016

Contributor

Don't understand how this is achieving that goal?

This comment has been minimized.

@aaronlehmann

aaronlehmann Jul 19, 2016

Collaborator

When an RPC is invoked, it first checks isRunning. isRunning checks for s.ctx to determine whether RPCs can run yet. So it's important to set s.ctx only after we initialize the relevant variables.

return store.UpdateCluster(tx, clusters[0])
}))

time.Sleep(500 * time.Millisecond)

This comment has been minimized.

@diogomonica

diogomonica Jul 19, 2016

Contributor

lame

@diogomonica

This comment has been minimized.

Copy link
Contributor

diogomonica commented Jul 19, 2016

LGTM

@aaronlehmann aaronlehmann force-pushed the aaronlehmann:join-tokens branch 3 times, most recently from 0429514 to 361afa3 Jul 19, 2016

@aaronlehmann

This comment has been minimized.

Copy link
Collaborator

aaronlehmann commented Jul 19, 2016

Added a test TestLoadOrCreateSecurityConfigWrongCAHash that ensures that we properly check the CA cert hash part of the token.

@tonistiigi

This comment has been minimized.

Copy link
Member

tonistiigi commented Jul 20, 2016

@aaronlehmann

Got this panic when shutting down a manager. I had done a rotation before that.

panic: could not find ':' in digest:

goroutine 84 [running]:
panic(0xee3c20, 0xc8203d7470)
    /usr/local/go/src/runtime/panic.go:464 +0x3e6
github.com/docker/swarmkit/vendor/github.com/docker/distribution/digest.Digest.sepIndex(0x0, 0x0, 0x6)
    /home/vagrant/data/go/src/github.com/docker/swarmkit/vendor/github.com/docker/distribution/digest/digest.go:135 +0xda
github.com/docker/swarmkit/vendor/github.com/docker/distribution/digest.Digest.Hex(0x0, 0x0, 0x0, 0x0)
    /home/vagrant/data/go/src/github.com/docker/swarmkit/vendor/github.com/docker/distribution/digest/digest.go:124 +0x37
github.com/docker/swarmkit/ca.GenerateJoinToken(0xc82028e4e0, 0x0, 0x0)
    /home/vagrant/data/go/src/github.com/docker/swarmkit/ca/config.go:169 +0x28c
github.com/docker/swarmkit/manager.(*Manager).Run.func2.1(0x7fae4666bf70, 0xc8208d7830, 0x0, 0x0)
    /home/vagrant/data/go/src/github.com/docker/swarmkit/manager/manager.go:318 +0x139
github.com/docker/swarmkit/manager/state/store.(*MemoryStore).update(0xc820276150, 0x7fae46669958, 0xc82008a480, 0xc82024ef18, 0x0, 0x0)
    /home/vagrant/data/go/src/github.com/docker/swarmkit/manager/state/store/memory.go:228 +0x178
github.com/docker/swarmkit/manager/state/store.(*MemoryStore).Update(0xc820276150, 0xc82024ef18, 0x0, 0x0)
    /home/vagrant/data/go/src/github.com/docker/swarmkit/manager/state/store/memory.go:270 +0x54
github.com/docker/swarmkit/manager.(*Manager).Run.func2(0xc8202e0cc0, 0xc820270100, 0x7fae46667518, 0xc82026ba80)
    /home/vagrant/data/go/src/github.com/docker/swarmkit/manager/manager.go:340 +0x48c
created by github.com/docker/swarmkit/manager.(*Manager).Run
    /home/vagrant/data/go/src/github.com/docker/swarmkit/manager/manager.go:440 +0x18f

Would be easier to test if swarmctl had a way to see/rotate tokens. Also I saw references to acceptance policies in swarmctl/api.

@aaronlehmann aaronlehmann force-pushed the aaronlehmann:join-tokens branch from 361afa3 to 7772e64 Jul 20, 2016

@aaronlehmann

This comment has been minimized.

Copy link
Collaborator

aaronlehmann commented Jul 20, 2016

I pushed a change that should fix the panic. The RootCA struct has a Digest field but it was not being set when we retrieved the cert from a remote CA. I think this field was not being used before this PR, so it was not being set consistently.

The acceptance policy stuff in api should be related to deprecated fields. I am removing the related items from swarmctl. I'll also add token display/rotation to swarmctl.

Replace secrets, node acceptance, and CA hash with join tokens
Implement the swarmkit parts of the flow described in
moby/moby#24430.

Signed-off-by: Aaron Lehmann <aaron.lehmann@docker.com>

@aaronlehmann aaronlehmann force-pushed the aaronlehmann:join-tokens branch from 7772e64 to dd9a504 Jul 20, 2016

@aaronlehmann

This comment has been minimized.

Copy link
Collaborator

aaronlehmann commented Jul 20, 2016

Removed acceptance policy stuff from swarmctl, added join tokens to to the swarmctl cluster inspect output, and added a --rotate-join-toker=(worker|manager) flag to swarmctl cluster update.

@tonistiigi

This comment has been minimized.

Copy link
Member

tonistiigi commented Jul 20, 2016

LGTM

@diogomonica

This comment has been minimized.

Copy link
Contributor

diogomonica commented Jul 20, 2016

@aaronlehmann failing CI on TestManager

@aaronlehmann

This comment has been minimized.

Copy link
Collaborator

aaronlehmann commented Jul 20, 2016

Looks unrelated - my latest changes were only to swarmctl. I started a new run.

@aaronlehmann

This comment has been minimized.

Copy link
Collaborator

aaronlehmann commented Jul 20, 2016

Tests passed this time.


generatedSecretEntropyBytes = 16
joinTokenBase = 36
// ceil(log(2^128-1, 36))

This comment has been minimized.

@diogomonica

diogomonica Jul 20, 2016

Contributor

repeated comment?

This comment has been minimized.

@aaronlehmann

aaronlehmann Jul 20, 2016

Collaborator

huh?

@diogomonica diogomonica merged commit d74b236 into docker:master Jul 20, 2016

3 checks passed

ci/circleci Your tests passed on CircleCI!
Details
codecov/project 55.35% (target 0.00%)
Details
docker/dco-signed All commits signed
Details

@aaronlehmann aaronlehmann deleted the aaronlehmann:join-tokens branch Jul 21, 2016

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