WIP: Client rotate remote key #553
Conversation
Signed-off-by: Ying Li <[email protected]>
This also changes GetKey and RotateKey to return a data.PublicKey, rather than bytes. Signed-off-by: Ying Li <[email protected]>
… rotating keys. Signed-off-by: Ying Li <[email protected]>
cyli
force-pushed
the
client-rotate-remote
branch
from
211686d to
a8ddcdf
Compare
…rvice and root pubkeys. So that the rotate request gets signed. Signed-off-by: Ying Li <[email protected]>
This also changes server key rotation behavior to just be get key behavior, until such time as the server key rotation behavior is implemented. We also now allow the client to remotely rotate timestmap keys. Signed-off-by: Ying Li <[email protected]>
cyli
force-pushed
the
client-rotate-remote
branch
from
a8ddcdf to
413e1cd
Compare
| @@ -107,12 +107,13 @@ func Test0Dot1RepoFormat(t *testing.T) { | |||
| // one and try to create a new one from scratch, which will be the wrong version | |||
| require.NoError(t, repo.fileStore.RemoveMeta(data.CanonicalTimestampRole)) | |||
|
|
|||
| // rotate the timestamp key, since the server doesn't have that one | |||
| // rotate the timestamp key, since the server doesn't have that one (we can't just | |||
Since it already does something like that to find signing keys for signing roles. Also, include some better comments, debug logging, and error reporting. Signed-off-by: Ying Li <[email protected]>
Signed-off-by: Ying Li <[email protected]>
|
|
||
| func (err ErrInvalidLocalRole) Error() string { | ||
| return fmt.Sprintf( | ||
| "notary does not support the client managing the %s key", err.Role) |
There was a problem hiding this comment.
Should the messaging be stronger? "notary does not permit ..."? Don't want people filing issues for things we never intend to support.
There was a problem hiding this comment.
I'd agree with @endophage for stronger messaging, since we similar language for the root rotation error message and we intend to support that
| default: | ||
| logger.Errorf("400 GET %s key: %v", role, err) | ||
| return errors.ErrInvalidRole.WithDetail(role) | ||
| key, err = snapshot.GetOrCreateSnapshotKey(s.gun, s.store, s.crypto, s.keyAlgo) |
There was a problem hiding this comment.
The default case seems to have dropped off? Should be maintained. There's nothing stopped a client (or even a curl request) from attempting to rotate the root, targets, or delegation keys.
There was a problem hiding this comment.
Ah, I see you moved it above to line 179. I'm not sure exactly what the implementation of server signing will look like but I feel like it's simpler to keep it here in the switch. Then manipulating which roles can be server side is better contained in this one place and adding/removing cases is sufficient to change which keys the server holds, and there isn't a conditional elsewhere to worry about.
There was a problem hiding this comment.
I don't mind moving it back, but I was going to have a rotateKeyHandler, which would need to perform the same check, so if we want it in a single location, should it be in parseKeys? https://github.com/docker/notary/pull/534/files#diff-b60e09f129d59867e23c3c6e1e94319cR159 was eventually supposed to be how this would look.
I can also just move it back to getKeyHandler for now, and when/if that change lands, move it to parseKeyParams
There was a problem hiding this comment.
Switches without a default make me look twice, and are more concerning when dealing with user input. Having the default here makes it obvious what the valid input is for this flow. Having the check in parseKeyParams leaves it open to dropping off if somebody isn't paying attention.
|
I don't see where the server is checking the client's signed request. Also seems like a partial server implementation that currently just returns the existing key (if there is one). That's fine if the server piece is coming separately but I'd like to see the signed request checking in this PR given that functionality is landing here. |
| if err == nil { | ||
| pubKey, err = remote.RotateKey(role, r.CryptoService, rootKeys...) | ||
| } else if _, ok := err.(signed.ErrNoKeys); ok { | ||
| err = fmt.Errorf("root signing key unavailable so unable to rotate key anyway") |
| @@ -21,6 +21,7 @@ import ( | |||
| "github.com/Sirupsen/logrus" | |||
| ctxu "github.com/docker/distribution/context" | |||
| "github.com/docker/go/canonical/json" | |||
| "github.com/gorilla/mux" | |||
There was a problem hiding this comment.
thanks for updating this 👍
- better comments - more test assertions - better error messages - stylistic improvements - variable renaming Signed-off-by: Ying Li <[email protected]>
|
@endophage Yep I was hoping to do the server part in a separate PR, because it's huge :( And this is already huge. Will be happy to add the server validation of the request, though - hopefully that shouldn't add to much. |
|
@cyli rebase needed. |
|
@diogomonica will do - I think in order to decrease the size of this I'm going to address the CLI changes here and #554 first in a separate PR :) |
|
Closing in favor of #889 |
This supercedes #529.
This adds the functionality for the client to rotate remote keys, including the timestamp key.
In order to do so, it must sign a request with the root key, in order to prove that it has the root key, and the rotation is published right away.
Part of addressing #347