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

Improve default key management experience #59

Merged
merged 3 commits into from
Apr 8, 2020

Conversation

trishankatdatadog
Copy link
Member

@trishankatdatadog trishankatdatadog commented Feb 14, 2020

  • Push snapshot key, just like timestamp, to Notary server.
  • Reuse targets key, just like root, across bundle repositories.
  • [ ] Delegate all bundles from targets to targets/releases` for better security posture.

@trishankatdatadog trishankatdatadog force-pushed the trishankatdatadog/cmd-to-make-keys branch 3 times, most recently from 6aecf97 to b952f28 Compare February 25, 2020 22:44
@trishankatdatadog trishankatdatadog force-pushed the trishankatdatadog/cmd-to-make-keys branch from 3a43bcb to 768503b Compare March 12, 2020 16:50
@trishankatdatadog trishankatdatadog marked this pull request as ready for review March 12, 2020 16:54
log.Debug("Nothing to do, only one targets key available")
case 2:
// First, we publish current changes to repository in order to list roles.
// FIXME: Find a find better way to list roles w/o publishing changes first.
Copy link
Member

Choose a reason for hiding this comment

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

Why exactly do we need to publish the changes first?

Copy link
Member

Choose a reason for hiding this comment

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

Also, could we have a bit more verbose logging here?

Copy link
Member Author

Choose a reason for hiding this comment

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

Unfortunately, without first publishing, we are unable to list roles, IIRC from experience. Perhaps @justincormack could help to confirm, here?

Also, what do you mean by verbose logging? Where would you like to see them?

Copy link
Member

@radu-matei radu-matei left a comment

Choose a reason for hiding this comment

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

This looks great!
The only comment I have is that this PR does diverge from the Docker CLI behaviour in a way we agree is helpful.
However, there might be scenarios where users might actively want to have different pairs of root and targets keys for different bundles, scenario that is not covered by this PR.

I don't think we should hold up this PR because of it, but it would be fantastic if users / implementors could have this choice.

What do you think?

Also, before merging, could you please clean-up the commits so we have a clean commit history?
Thanks!

@trishankatdatadog
Copy link
Member Author

The only comment I have is that this PR does diverge from the Docker CLI behaviour in a way we agree is helpful.
However, there might be scenarios where users might actively want to have different pairs of root and targets keys for different bundles, scenario that is not covered by this PR.

I don't think we should hold up this PR because of it, but it would be fantastic if users / implementors could have this choice.

Agreed. I will file an issue to allow users to use different pairs of root and targets keys.

Also, before merging, could you please clean-up the commits so we have a clean commit history?

Sorry, could you clarify this?

@radu-matei
Copy link
Member

I mean that there are a few commits in this PR that, if they ended up in the main branch commit history, wouldn't add that much, but significantly pollute the overall history.

So ideally, you would squash some commits and only keep the logical commits that make sense. (And maybe rewrite their commit messages.)

(It is a sensible middle ground between squashing all commits into one, and keeping all of them),

Signed-off-by: Trishank K Kuppusamy <trishank.kuppusamy@datadoghq.com>
Signed-off-by: Trishank K Kuppusamy <trishank.kuppusamy@datadoghq.com>
Signed-off-by: Trishank Karthik Kuppusamy <trishank.kuppusamy@datadoghq.com>
@trishankatdatadog trishankatdatadog force-pushed the trishankatdatadog/cmd-to-make-keys branch from 30b5c5e to dad7da0 Compare April 8, 2020 18:51
@trishankatdatadog
Copy link
Member Author

Squashed commits! Hopefully much cleaner.

@radu-matei
Copy link
Member

This is excellent, thank you so much!

@radu-matei radu-matei merged commit 9400205 into master Apr 8, 2020
@radu-matei radu-matei deleted the trishankatdatadog/cmd-to-make-keys branch April 8, 2020 20:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants