-
Notifications
You must be signed in to change notification settings - Fork 562
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
interfaces: new helpers to get and compare system key, for use with seeding debug api (1/N) #9028
interfaces: new helpers to get and compare system key, for use with seeding debug api (1/N) #9028
Conversation
…p debug seeding`.
// TODO: write custom struct compare | ||
return !reflect.DeepEqual(mySystemKey, &diskSystemKey), nil | ||
return reflect.DeepEqual(systemKey1, systemKey2), 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.
Note: this might look very confusing with unified diff, but see line 255 above.
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
interfaces/system_key.go
Outdated
raw, err := ioutil.ReadFile(dirs.SnapSystemKeyFile) | ||
if err != nil { | ||
if os.IsNotExist(err) { | ||
return false, ErrSystemKeyMissing |
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.
s/false/nil/ ?
interfaces/system_key.go
Outdated
|
||
var diskSystemKey systemKey | ||
if err := json.Unmarshal(raw, &diskSystemKey); err != nil { | ||
return false, err |
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.
same
interfaces/system_key.go
Outdated
_, ok1 := systemKey1.(*systemKey) | ||
_, ok2 := systemKey2.(*systemKey) | ||
if !(ok1 && ok2) { | ||
return false, fmt.Errorf("internal error: cannot compare system keys") |
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.
"MatchSystemKeys: arguments are not system keys" ?
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! Some ideas inline
|
||
ok, err := interfaces.MatchSystemKeys(key1, key2) | ||
c.Assert(err, IsNil) | ||
c.Check(ok, Equals, false) |
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 we add a test that checks if two systems keys are equal? AFAICT we don't test this right now?
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.
Right, thanks for catching, forgot about this one!
|
||
// RecordedSystemKey returns opaque system key read from the disk. | ||
func RecordedSystemKey() (interface{}, error) { | ||
raw, err := ioutil.ReadFile(dirs.SnapSystemKeyFile) |
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.
(nitpick) we could share the read logic between RecordedSystemKey and SystemKeyMismatch with something like:
diff --git a/interfaces/system_key.go b/interfaces/system_key.go
index 2988db52f3..2b9989ccf2 100644
--- a/interfaces/system_key.go
+++ b/interfaces/system_key.go
@@ -214,17 +214,10 @@ func SystemKeyMismatch() (bool, error) {
return false, err
}
- raw, err := ioutil.ReadFile(dirs.SnapSystemKeyFile)
- if err != nil && os.IsNotExist(err) {
- return false, ErrSystemKeyMissing
- }
+ diskSystemKey, err := readSystemKey()
if err != nil {
return false, err
}
- var diskSystemKey systemKey
- if err := json.Unmarshal(raw, &diskSystemKey); err != nil {
- return false, err
- }
// deal with the race that "snap run" may start, then snapd
// is upgraded and generates a new system-key with different
// inputs than the "snap run" in memory. In this case we
@@ -252,28 +245,34 @@ func SystemKeyMismatch() (bool, error) {
diskSystemKey.AppArmorParserFeatures = nil
mySystemKey.AppArmorParserFeatures = nil
- ok, err := MatchSystemKeys(mySystemKey, &diskSystemKey)
+ ok, err := MatchSystemKeys(mySystemKey, diskSystemKey)
return !ok, err
}
-// RecordedSystemKey returns opaque system key read from the disk.
-func RecordedSystemKey() (interface{}, error) {
+func readSystemKey() (*systemKey, error) {
raw, err := ioutil.ReadFile(dirs.SnapSystemKeyFile)
+ if err != nil && os.IsNotExist(err) {
+ return nil, ErrSystemKeyMissing
+ }
if err != nil {
- if os.IsNotExist(err) {
- return nil, ErrSystemKeyMissing
- }
return nil, err
}
-
var diskSystemKey systemKey
if err := json.Unmarshal(raw, &diskSystemKey); err != nil {
return nil, err
}
-
return &diskSystemKey, nil
}
+// RecordedSystemKey returns opaque system key read from the disk.
+func RecordedSystemKey() (interface{}, error) {
+ diskSystemKey, err := readSystemKey()
+ if err != nil {
+ return nil, err
+ }
+ return diskSystemKey, nil
+}
+
// CurrentSystemKey calculates and returns opaque system key.
func CurrentSystemKey() (interface{}, error) {
currentSystemKey, err := generateSystemKey()
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.
LGTM with one nitpick about helper name.
interfaces/system_key.go
Outdated
} | ||
|
||
// MatchSystemKeys compares opaque system keys | ||
func MatchSystemKeys(systemKey1, systemKey2 interface{}) (bool, error) { |
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.
Nitpick on the name, since this compares system keys for equality, how about just SystemKeysEqual
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.
+1, did some small adjustments
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.
+1
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.
LGTM
New helpers to get and compare opaque system keys, for use in debug api for seeding and preseeding.