-
Notifications
You must be signed in to change notification settings - Fork 17
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
[2/3] Keydata v3 scope changes #270
[2/3] Keydata v3 scope changes #270
Conversation
7833c04
to
2f4124c
Compare
2f4124c
to
9de36fb
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for working on this - I've done a first pass over this, with the exception of the test changes in the core package.
bootenv/env.go
Outdated
currentBootMode atomic.Value | ||
) | ||
|
||
var SetModel = func(model secboot.SnapModel) bool { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rather than making this function and SetBootMode
mockable, could you just export loadCurrentModel
and loadCurrentBootMode
for testing in export_test.go so that you can make use of the real function and still be able to read the value back.
These probably want some documentation as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The initial branch only allowed currentModel
and currentBootMode
to be updated once (using CompareAndSwap(nil, model)
) so I couldn't change their values between tests. Does that need to change or did I miss something here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, so the intention was that these functions would only succeed the first time that they are called. It might be better to just convert these to call atomic.Value.Store
instead and make them return nothing.
You'd still want a way to clear them before a test though, so perhaps adding a function to export_test.go
to just set both currentModel
and currentBootMode
to zero values would be ok. With this, you wouldn't need to be able to mock the functions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still can't see how I can workaround the fact that atomic.Value can't be set to nil after it is set. My intention here was to mock both the setters and getters to use keyDataPlatformSuite
variables instead of the real ones.
I tried changing the setters to use atomic.Value.Store
then have a new function ClearModelAndBootMode
in export_test.go
but atomic.value.Store
, atomic.value.Swap
and atomic.value.CompareAndSwap
operations fail when trying to set a nil value to an atomic variable that has been set.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, when I suggested just setting currentModel
and currentBootMode
to zero values, I meant not doing it atomically and just doing something like this:
currentModel = atomic.Value{}
currentBootMode = atomic.Value{}
This obviously isn't safe in production code, but should be fine from test code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah right, thanks for this suggestion, it cleaned up the code quite a bit. I implemented it as suggested.
crypt.go
Outdated
@@ -430,7 +430,10 @@ func ActivateVolumeWithKeyData(volumeName, sourceDevicePath string, authRequesto | |||
return errors.New("invalid RecoveryKeyTries") | |||
} | |||
if options.Model == nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To simplify how snap-bootstrap
uses this API, it might be worth getting rid of this option and instead obtaining the model the same way that the code in bootenv.KeyDataScope.IsBootEnvironmentAuthorized
obtains it. That would probably require moving the currentModel
and currentBootMode
global variables to their own internal package that both this and the bootenv package can depend on.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That sounds like a nice cleanup. Separate PR maybe? I will need to check how snapd currently uses this API. So how that would look like from a caller's standpoint, they would need to run SetModel
first to set the current model right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A separate PR sounds fine. Yes, snap-bootstrap would call SetModel
and that would delegate to an internal package containing the actual global variables and loadCurrentModel
etc, which could be consumed by both public packages.
bootenv/keydata_test.go
Outdated
validModes: validModes, | ||
bootMode: invalidBootMode, | ||
}), ErrorMatches, "unauthorized boot mode") | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whilst the test coverage here is mostly ok (other than missing the JSON encoding / decoding), it's important to have a test that verifies a specific primary key always derives the same elliptic key - I'll leave it up to you to decide how best to test that, but it will require some hard-coded keys in the test code. There are some examples of this kind of test already (eg, policyV3SuiteNoTPM.TestDerivePolicyAuthKey*
).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a test for that too. While adding the tests for marshalling/unmarshalling, I accidentally came across the fact that Go's ecdsa.GenerateKey
is not deterministic. After adding the test I modified KeyDataScope.deriveSigner()
to use ecdsa.GenerateKey()
instead of the internal crypto out of curiosity and my test failed all the times I ran it (I am running 10 iterations).
Interestingly many of the times it failed even before the test reached my loop, between the 2 derivations that happen in NewKeyDataScope
itself (one when filling the PublicKey field and one in authorize()
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. Yes, go1.20 changed the behaviour of ecdsa.GenerateKey
to use a non-deterministic method to generate the key, which is a bit of a pain.
The new tests do check that multiple calls to KeyDataScope.deriveSigner
produce the same key, but there also needs to be a test that verifies it produces the expected key for a fixed input (ie, a test with a hardcoded primary key and expected derived key).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added 2 tests for 2 different fixed keys and another one that tests that different keys are derived from the same primary key but a different role which was also missing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've done another pass and left some comments on test code.
I do think the overall approach for testing here probably needs some tweaks though - I haven't left comments in every place, but I've got some high level notes:
- In general, tests should only be skipped (with
check.C.Skip
) when some pre-requisites are not met (ie, there's something missing in the environment) or if a test is known to fail. It shouldn't be used as a way to exit early from tests to avoid running some combinations in the way that's happening here (ie,if s.Version == 1
) as it creates entries in the test log that are misleading. The tests should just avoid running invalid combinations entirely. - I think we should avoid test functions that are used for new and legacy combinations. I would probably update existing test functions to work with the new format (obviously removing the parts and tests that are redundant now), and then add some additional functions just to test some legacy format keys - these can just be a copy of a small subset of existing test code - ie, can we read / unlock / write an existing key, does the legacy snap model authorization work, and can we update the models ok?
- For testing legacy format keys, there is some advantage to just embedding JSON in the tests as opposed to adding code and mocking functions to make it possible to create legacy keys. Running tests on embedded key data provides better guarantees that we don't break them.
Hopefully that makes sense :)
keydata_test.go
Outdated
restore, err := MockMakeDiskUnlockKey(primaryKey) | ||
c.Assert(err, IsNil) | ||
s.AddCleanup(restore) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This block looks out of place here - installing mocks should normally happen once as part of the test setup.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't exist anymore, all this functionality got refactored and there is no need to mock these now.
@chrisccoulson so regarding your general comment, I reverted the 2 commits that introduced the mixed new/legacy logic for the tests and rewrote them from scratch. I also addressed all of the inline comments. |
3870268
to
622643d
Compare
622643d
to
37adf33
Compare
Under the new design, the platform agnostic API provides a new role field so each key's use can be identified by snapd. This role field will be authenticated "indirectly" for the case of v3 keys by binding it to the authorization policy of the sealed object.
…odels Under the new design, the mechanism used to restrict access to confidential keys to a specific set of authorized models will be moved out of the common keyData implementation and into platform implementations that require it. As an example of the opposite, UEFI+TPM platforms use measured boot already to enforce restrictions based on snap models. AuthorizedSnapModels field is made optional and relevant functions are moved to keydata_legacy.go. PrimaryKey is no longer an argument of NewKeyData as it is was relevant for deriving the authKey which was used for checking authorized snap models. It is now moved to NewKeyDataScope under bootenv. SnapModelAuthKey*() functions have been renamed to SnapModelHMACKey*() to better reflect the legacy HMAC behavior.
under the new design, the cross-platform API is no longer responsible for authorizing snap models. This functionality is relevant for specific platforms that don't use measured boot and therefore has been moved to a separate package.
since for v2 keys model authorization operations are responsibility of each platform, the default behaviour should be skipping any model checking.
the digest algorithm used for creating the model digests needs to be initialized when creating a new keyDataScope object otherwise SetAuthorizedSnapModels() fails.
existing tests were modified to work for the new keydata format: 1) the bulk of the tests was modified to remove any code related to model authorization operations. 2) several tests that only applied for legacy keys were renamed and modified to test hardcoded legacy key scenarios.
bootenv/keydata.go: - additionalData -> AdditionalData - new unmarshalHashAlg() to unmarshal to hashAlg from ASN1. bootenv/export_test.go: - UnmarshalAdditionalData() to unmarshal to AdditionalData from ASN1. - KeyDataScope.TestSetVersion() to allow setting the version of key data scope objects in tests. - KeyDataScope.TestMatch() to compare the unmarshalled key identifier field to the expected derived one. bootenv/keydata_test.go: - add more complete test for MakeAdditionalData()
…ling/unmarshalling
to ensure that any future changes don't break backwards compatibility.
as new gen 2 keys break passphrase support in a non backwards compatible way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks, replied to some comment threads as well, tentatively approving but would be good to add package comments to bootenv and possibly rename it
keydata_test.go
Outdated
type testSnapModelAuthData struct { | ||
alg crypto.Hash | ||
authModels []SnapModel | ||
func (s *keyDataSuite) TestSnapModelAuthErrorHandling(c *C) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should this test name also contain Legacy?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I renamed it and moved it further down under the legacy tests section.
bootenv/keydata_test.go
Outdated
signer, err := kds.DeriveSigner(primaryKey, role) | ||
c.Assert(err, IsNil) | ||
|
||
prevKey, ok := signer.(*ecdsa.PrivateKey) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this meant to be privKey
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, fixed.
bootenv/keydata.go
Outdated
// MakeAdditionalData constructs the additional data that need to be integrity protected for | ||
// a key data scope (in AES-GCM for example). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think MakeAEADAdditionalData
is probably ok, given that it's expected that another package uses an implementation of cipher.AEAD
and consumes this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, I think this looks ok now - I left another small comment about a variable name
Original commit: chrisccoulson@87d9aa3
I will split the commit above in 3 separate cascading PRs and add tests for it:
This PR is rebased on top of 1. It adds the changes that split snap model authorization (and boot mode authorization) in a separate package (bootenv) which will now reside outside the cross-platform API.