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

asserts: add support for account-key constraints #12988

Merged
merged 2 commits into from
Aug 8, 2023

Conversation

pedronis
Copy link
Collaborator

and take them into account when verifying assertion signatures

and take them into account when verifying assertion signatures
@pedronis pedronis requested a review from mvo5 July 17, 2023 14:12
@mvo5 mvo5 requested a review from Meulengracht July 28, 2023 18:55
Copy link
Member

@Meulengracht Meulengracht left a comment

Choose a reason for hiding this comment

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

Looks really good, just two small suggestions

Comment on lines +276 to +283
t, ok := hm["type"]
if !ok {
return nil, fmt.Errorf("type header constraint mandatory in asserions constraints")
}
tstr, ok := t.(string)
if !ok {
return nil, fmt.Errorf("type header constraint must be a string")
}
Copy link
Member

Choose a reason for hiding this comment

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

checkNotEmptyString?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

the issue doing that would be less specific errors

asserts/account_key.go Outdated Show resolved Hide resolved
Copy link
Member

@olivercalder olivercalder left a comment

Choose a reason for hiding this comment

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

Looks great to me, just one tiny nitpick. Thank you!

}

func accountKeyFormatAnalyze(headers map[string]interface{}, body []byte) (formatnum int, err error) {
formatnum = 0
Copy link
Member

Choose a reason for hiding this comment

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

(very nitpick): s/formatnum/formatNum ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

historically the code uses formatnum consistently

Copy link
Contributor

@mvo5 mvo5 left a comment

Choose a reason for hiding this comment

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

Thank you, this looks good. But worth checking the suggestions from Philip I think.

asserts/database_test.go Outdated Show resolved Hide resolved
@pedronis pedronis merged commit 6cc7429 into canonical:master Aug 8, 2023
@pedronis pedronis added this to the 2.61 milestone Aug 8, 2023
@pedronis pedronis deleted the ak-constraints branch September 5, 2023 12:49
alexmurray pushed a commit to alexmurray/snapd that referenced this pull request Oct 17, 2023
and take them into account when verifying assertion signatures
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.

4 participants