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

Pass expiry to generate-envelope plugin #211

Merged
merged 1 commit into from
Nov 29, 2022
Merged

Pass expiry to generate-envelope plugin #211

merged 1 commit into from
Nov 29, 2022

Conversation

priteshbandi
Copy link
Contributor

@priteshbandi priteshbandi commented Nov 23, 2022

Pass expiry to envelope generator plugin so that it can be used for generating signature.
Issue: notaryproject/notation#443

Signed-off-by: Pritesh Bandi pritesb@amazon.com

Signed-off-by: Pritesh Bandi <pritesb@amazon.com>
Copy link
Contributor

@gokarnm gokarnm left a comment

Choose a reason for hiding this comment

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

LGTM!

@@ -375,6 +375,9 @@ All request attributes are required.
// Optional plugin configuration, map of string-string
"pluginConfig" : { },

// Optional signature expiry duration in seconds, number
"expiryDurationInSeconds" : 1234567,
Copy link
Contributor

Choose a reason for hiding this comment

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

Taking a look at the current notation.SignOptions, the expriy is a time.Time and not a time.Duration. Therefore, this field should also be a time field.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In notation cli user pass expiry as duration 24h5m, so notaiton.SignOptions should use time.Duration instead of time.Time because actual expiry should be calculated as close as possible to sign operation.

Scenario: user called notation sign $Image -e 1m with plugin and if the plugin execute took 30 secs, the user got the expiry only of 30 sec not 1 minute.

Copy link
Contributor

Choose a reason for hiding this comment

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

This is exactly I am worried about. The expiry should be deterministic and is not based on the signature generation time and influenced by any possible time skew.

For example, I want to sign an artifact for deployment and let it expire immediately after Thanksgiving 12:00AM sharp time. Therefore, I calculate the duration based on my local time and run notation sign. The signing process takes 30s and due a time skew between my machine and the server, the signature will actually be expired on the Thanksgiving Day 11:55 PM.

Therefore, expiry should be an exact date time for all internal component communication. The duration-typed expiry at the notation CLI side is for user experience, which can be reviewed later.

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we should remove expiry from CLI for now, if we didn't think through the behavior. Once it is released, it's a breaking change if we want to change the behaviors.

specs/plugin-extensibility.md Show resolved Hide resolved
Copy link
Contributor

@shizhMSFT shizhMSFT left a comment

Choose a reason for hiding this comment

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

LGTM as discussed in the community meeting that expiry in the signing options should be duration all of the places.

@priteshbandi priteshbandi merged commit f1691a6 into notaryproject:main Nov 29, 2022
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.

5 participants