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

[2/3] Keydata v3 scope changes #270

Merged
Merged
Show file tree
Hide file tree
Changes from 20 commits
Commits
Show all changes
49 commits
Select commit Hold shift + click to select a range
74f8b08
platform.go: add role field to PlatformKeyData
sespiros Oct 16, 2023
4645ce8
keydata.go, keydata_legacy.go: refactor handling of authorized snap m…
sespiros Oct 16, 2023
ed975b3
keydata_test.go: fix mockProtectKeys after scope changes
sespiros Jan 23, 2024
a76f332
bootenv: move keydata model authorization to separate package
sespiros Oct 16, 2023
cc16a83
keydata.go: add role field in secboot.keydata
sespiros Oct 16, 2023
1af77a7
crypt.go: make SkipSnapModelCheck the default behaviour
sespiros Jan 23, 2024
bfe8980
keydata_test.go: fix legacy test for the new role field
sespiros Oct 27, 2023
6c9fb0f
bootenv/keydata.go: initialize model digest hash algorithm
sespiros Oct 27, 2023
84e6d65
bootenv/keydata.go: add kdfAlg, baseVersion fields in additionalData
sespiros Oct 27, 2023
48fab3f
bootenv: add tests for keyDataScope
sespiros Oct 27, 2023
a214639
bootenv/keydata.go: add doc comments
sespiros Jan 16, 2024
78e3364
crypt_test.go: rename primaryKey to diskUnlockKey
sespiros Jan 17, 2024
a6e3520
crypt_test.go, keydata*test.go: modify tests for new keydata format
sespiros Jan 22, 2024
f9a7f8e
bootenv: add tests for MakeAdditionalData
sespiros Jan 22, 2024
8810ae5
snap_test.go: add test for computeSnapModelHash
sespiros Jan 23, 2024
dad82f9
bootenv/keydata_test.go: add tests for hashAlg/ecdsaPublicKey marshal…
sespiros Jan 23, 2024
d9b9a2a
bootenv/keydata_test.go: add a test for wrong key supplied to SetAuth…
sespiros Jan 23, 2024
2e4c29a
bootenv/keydata_test.go: add a test for wrong key supplied to SetAuth…
sespiros Jan 23, 2024
f2ffc65
bootenv/keydata_test.go: add test for deterministic derivation of ell…
sespiros Jan 23, 2024
2f0e295
bootenv: add scope tests
sespiros Jan 31, 2024
4c10b40
bootenv/keydata_test.go: add deriveSigner tests with fixed keys
sespiros Feb 27, 2024
0521626
keydata_legacy.go: rename primaryKey to auxKey in unmarshalV1KeyPayload
sespiros Feb 27, 2024
6625be3
keydata.go: expand doc comment for AuthorizedSnapModels
sespiros Feb 27, 2024
767ac13
bootenv: make additionalData private
sespiros Feb 27, 2024
dae8903
bootenv/keydata.go: expand MakeAdditionalData comment
sespiros Feb 27, 2024
d42e414
bootenv: move unmarshalHashAlg to export_test.go
sespiros Feb 27, 2024
8e77c31
bootenv: change baseVersion to generation
sespiros Feb 27, 2024
b1e8248
bootenv/keydata.go: add doc comments for KeyDataScopeParams fields
sespiros Feb 27, 2024
8299964
multiple: fix wording for comments mentioning ASN1
sespiros Feb 27, 2024
8cec665
keydata_test.go: add extra checks in TestNewKeyDataScopeSuccess
sespiros Feb 27, 2024
8177fbc
bootenv/keydata_test.go: add small style change
sespiros Feb 27, 2024
6bc4186
bootenv: move NewPrimaryKey to keydata_test.go
sespiros Feb 27, 2024
da49e74
bootenv: convert MakeAdditionalData tests to fixed
sespiros Feb 27, 2024
f65f537
bootenv/keydata_test.go: add extra checks for TestSetAuthorizedSnapMo…
sespiros Feb 27, 2024
0cd840a
bootenv/keydata_test.go: cleanup tests
sespiros Feb 27, 2024
159895f
bootenv/keydata_test.go: consistent variable names in tests
sespiros Feb 27, 2024
e6408fd
bootenv/keydata_test.go: add extra checks for TestSetAuthorizedBootModes
sespiros Feb 27, 2024
284ebca
keydata_test.go: rename diskUnlockKey to unlockKey
sespiros Feb 27, 2024
2a5277c
keydata_test.go: remove TestRecoverLegacyKeyWithPassphrase
sespiros Feb 27, 2024
c12c559
keydata_test.go: add TestChangePassphraseWrongPassphrase again
sespiros Feb 27, 2024
6051c17
bootenv: refactor use of currentBootMode and currentModel in tests
sespiros Feb 27, 2024
8f2f770
bootenv: rename to bootscope
sespiros Feb 27, 2024
27bdb5f
bootscope: rename env.go to scope.go and add package doc comment
sespiros Feb 27, 2024
65291ec
bootscope: rename MakeAdditionalData to MakeAEADAdditionalData
sespiros Feb 27, 2024
07bab29
bootscope/export_test.go: remove unmarshalHashAlg
sespiros Feb 27, 2024
6a0df9b
bootscope/keydata_test.go: remove tests for mockPlatformKeyDataHandler
sespiros Feb 27, 2024
29521d8
keydata_test.go: move and rename TestSnapModelAuthErrorHandling
sespiros Feb 27, 2024
576b3cf
bootscope/keydata_test.go: fix typo
sespiros Feb 27, 2024
7957f52
bootscope/keydata: add marshal and unmarshal JSON for KeyDataScope
sespiros Feb 28, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 28 additions & 0 deletions bootenv/bootenv_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
// -*- Mode: Go; indent-tabs-mode: t -*-

/*
* Copyright (C) 2023 Canonical Ltd
*
* This program is free software: you can redistribute it and/or modify
* it under the terms of the GNU General Public License version 3 as
* published by the Free Software Foundation.
*
* This program is distributed in the hope that it will be useful,
* but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
* GNU General Public License for more details.
*
* You should have received a copy of the GNU General Public License
* along with this program. If not, see <http://www.gnu.org/licenses/>.
*
*/

package bootenv

import (
"testing"

. "gopkg.in/check.v1"
)

func Test(t *testing.T) { TestingT(t) }
56 changes: 56 additions & 0 deletions bootenv/env.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,56 @@
// -*- Mode: Go; indent-tabs-mode: t -*-

/*
* Copyright (C) 2023 Canonical Ltd
*
* This program is free software: you can redistribute it and/or modify
* it under the terms of the GNU General Public License version 3 as
* published by the Free Software Foundation.
*
* This program is distributed in the hope that it will be useful,
* but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
* GNU General Public License for more details.
*
* You should have received a copy of the GNU General Public License
* along with this program. If not, see <http://www.gnu.org/licenses/>.
*
*/

package bootenv
Copy link
Collaborator

Choose a reason for hiding this comment

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

the package itself probably needs a doc comment and maybe a clearer name, I'm not sure why it's called bootenv? it exists to offer key scope support for platforms not doing measured boot?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I guess bootenv was the initial name because the key scope support is related to the boot environment (boot mode and models). Maybe a different name with the word "scope" in it? bootscope/keyscope? @chrisccoulson

Copy link
Collaborator

Choose a reason for hiding this comment

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

bootscope is not a bad name, adding a package doc comment is still also a good idea, as this is meant to be used by platforms that don't do measurements?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm fine with bootscope as well.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Renamed bootenv to bootscope and added package doc comment.


import (
"errors"
"sync/atomic"

"github.com/snapcore/secboot"
)

var (
currentModel atomic.Value
currentBootMode atomic.Value
)

var SetModel = func(model secboot.SnapModel) bool {
Copy link
Collaborator

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.

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 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?

Copy link
Collaborator

@chrisccoulson chrisccoulson Feb 23, 2024

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.

Copy link
Collaborator Author

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.

Copy link
Collaborator

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.

Copy link
Collaborator Author

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.

return currentModel.CompareAndSwap(nil, model)
}

var SetBootMode = func(mode string) bool {
return currentBootMode.CompareAndSwap(nil, mode)
}

var loadCurrentModel = func() (secboot.SnapModel, error) {
model, ok := currentModel.Load().(secboot.SnapModel)
if !ok {
return nil, errors.New("SetModel hasn't been called yet")
}
return model, nil
}

var loadCurrentBootMode = func() (string, error) {
mode, ok := currentBootMode.Load().(string)
if !ok {
return "", errors.New("SetBootMode hasn't been called yet")
}
return mode, nil
}
159 changes: 159 additions & 0 deletions bootenv/export_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,159 @@
// -*- Mode: Go; indent-tabs-mode: t -*-

/*
* Copyright (C) 2023 Canonical Ltd
*
* This program is free software: you can redistribute it and/or modify
* it under the terms of the GNU General Public License version 3 as
* published by the Free Software Foundation.
*
* This program is distributed in the hope that it will be useful,
* but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
* GNU General Public License for more details.
*
* You should have received a copy of the GNU General Public License
* along with this program. If not, see <http://www.gnu.org/licenses/>.
*
*/

package bootenv

import (
"bytes"
"crypto"
"crypto/ecdsa"
"crypto/elliptic"
"crypto/rand"
"crypto/x509"
"errors"

"github.com/snapcore/secboot"
internal_crypto "github.com/snapcore/secboot/internal/crypto"
"golang.org/x/crypto/cryptobyte"
cryptobyte_asn1 "golang.org/x/crypto/cryptobyte/asn1"
)

var (
ComputeSnapModelHash = computeSnapModelHash
chrisccoulson marked this conversation as resolved.
Show resolved Hide resolved
)

func MockSetModel(f func(secboot.SnapModel) bool) (restore func()) {
origSetModel := SetModel
SetModel = f
return func() {
SetModel = origSetModel
}
}

func MockSetBootMode(f func(string) bool) (restore func()) {
origSetBootMode := SetBootMode
SetBootMode = f
return func() {
SetBootMode = origSetBootMode
}
}

func MockLoadCurrentModel(f func() (secboot.SnapModel, error)) (restore func()) {
origLoadCurrentModel := loadCurrentModel
loadCurrentModel = f
return func() {
loadCurrentModel = origLoadCurrentModel
}
}

func MockLoadCurrenBootMode(f func() (string, error)) (restore func()) {
origLoadCurrentBootMode := loadCurrentBootMode
loadCurrentBootMode = f
return func() {
loadCurrentBootMode = origLoadCurrentBootMode
}
}

func (d *KeyDataScope) TestSetVersion(version int) {
d.data.Version = version
}

func UnmarshalAdditionalData(data []byte) (*AdditionalData, error) {
s := cryptobyte.String(data)

if !s.ReadASN1(&s, cryptobyte_asn1.SEQUENCE) {
return nil, errors.New("malformed input")
}

aad := new(AdditionalData)

if !s.ReadASN1Integer(&aad.Version) {
return nil, errors.New("malformed version")
}

if !s.ReadASN1Integer(&aad.BaseVersion) {
return nil, errors.New("malformed base version")
}

kdfAlg, err := unmarshalHashAlg(&s)
if err != nil {
return nil, errors.New("malformed kdf")
}
aad.KdfAlg = kdfAlg

var authMode int
if !s.ReadASN1Enum(&authMode) {
return nil, errors.New("malformed Auth mode")
}
aad.AuthMode = secboot.AuthMode(authMode)

keyIdAlg, err := unmarshalHashAlg(&s)
if err != nil {
return nil, errors.New("malformed kdf")
}
aad.KeyIdentifierAlg = keyIdAlg

if !s.ReadASN1Bytes(&aad.KeyIdentifier, cryptobyte_asn1.OCTET_STRING) {
return nil, errors.New("malformed Key identifier")
}

return aad, nil
}

func (d *KeyDataScope) TestMatch(KDFAlg crypto.Hash, keyIdentifier []byte) bool {
der, err := x509.MarshalPKIXPublicKey(d.data.PublicKey.PublicKey)
if err != nil {
return false
}

h := KDFAlg.New()
h.Write(der)
return bytes.Equal(h.Sum(nil), keyIdentifier)
}

func (d *KeyDataScope) DeriveSigner(key secboot.PrimaryKey, role string) (crypto.Signer, error) {
return d.deriveSigner(key, role)
}

func NewHashAlg(alg crypto.Hash) hashAlg {
return hashAlg(alg)
}

func NewEcdsaPublicKey(rand []byte) (ecdsaPublicKey, error) {
var pk ecdsaPublicKey

privateKey, err := internal_crypto.GenerateECDSAKey(elliptic.P256(), bytes.NewReader(rand))
if err != nil {
return pk, err
}

pk.PublicKey = privateKey.Public().(*ecdsa.PublicKey)

return pk, nil
}

func NewPrimaryKey(sz1 int) (secboot.PrimaryKey, error) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

It doesn't look like this relies on anything unexported from this package, so it could be implemented entirely in the test package instead (inside keydata_test.go).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed.

primaryKey := make(secboot.PrimaryKey, sz1)
_, err := rand.Read(primaryKey)
if err != nil {
return nil, err
}

return primaryKey, nil
}
Loading
Loading