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

client: update expiration check to match spec #1235

Merged

Conversation

joshuagl
Copy link
Member

@joshuagl joshuagl commented Dec 8, 2020

Fixes #1231

Description of the changes being introduced by the pull request:

The specification, as of 1.0.16, describes an update expiration check as:

The expiration timestamp in the trusted $ROLE metadata file MUST be
higher than the fixed update expiration time.

Having done some research into how other security providers are comparing
expiration equivalents (i.e. OpenSSL x509 certificate checking code, and
GnuPG expiration checks), and how other TUF implementations are performing
the same check (rust-tuf, go-tuf), we came to a consensus that the correct
way to implement expiration comparisons is:

expiration <= now

Where:
expiration: is the metadata's expiration datetime
now: is the current system time, or the fixed notion of time in the
detailed client workflow (introduced in 1.0.16 of the spec)

Please verify and check that the pull request fulfills the following
requirements
:

  • The code follows the Code Style Guidelines
  • Tests have been added for the bug fix or new feature
  • Docs have been added for the bug fix or new feature

The specification, as of 1.0.16, describes an update expiration check as:

> The expiration timestamp in the trusted $ROLE metadata file MUST be
  higher than the fixed update expiration time.

Having done some research into how other security providers are comparing
expiration equivalents (i.e. OpenSSL x509 certificate checking code, and
GnuPG expiration checks), and how other TUF implementations are performing
the same check (rust-tuf, go-tuf), we came to a consensus that the correct
way to implement expiration comparisons is:

    expiration <= now

Where:
  expiration: is the metadata's expiration datetime
  now: is the current system time, or the fixed notion of time in the
       detailed client workflow (introduced in 1.0.16 of the spec)

Fixes theupdateframework#1231

Signed-off-by: Joshua Lock <jlock@vmware.com>
Copy link
Member

@trishankatdatadog trishankatdatadog left a comment

Choose a reason for hiding this comment

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

LGTM, thanks, Joshua!

Could we quickly mock some tests, please, if that's not too much to ask?

Might also help to test we're handling UTC correctly, it was not obvious to me from reading the code...

@joshuagl
Copy link
Member Author

joshuagl commented Dec 9, 2020

Could we quickly mock some tests, please, if that's not too much to ask?

Done in follow-on commit fccd078

Might also help to test we're handling UTC correctly, it was not obvious to me from reading the code...

I'm not sure the test I added is doing this, could you elaborate a bit more on what your concern is? Perhaps we can add a test to check.

Add a test to ensure that metadata expires at the expiration time, not
after it.
This tests the change to the updater introduced in 4bcd703

Signed-off-by: Joshua Lock <jlock@vmware.com>
@trishankatdatadog
Copy link
Member

I'm not sure the test I added is doing this, could you elaborate a bit more on what your concern is? Perhaps we can add a test to check.

I think your tests cover <, ==, and >, so we should be OK.

Copy link
Collaborator

@MVrachev MVrachev left a comment

Choose a reason for hiding this comment

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

LGTM!

@lukpueh lukpueh merged commit b2e3c83 into theupdateframework:develop Dec 11, 2020
@joshuagl joshuagl deleted the joshuagl/expiration-check branch December 11, 2020 14:32
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.

Expiration check does not follow the spec
6 participants