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

go.mod: move to latest secboot #13339

Conversation

valentindavid
Copy link
Contributor

No description provided.

@github-actions github-actions bot added the Run Nested -auto- Label automatically added in case nested tests need to be executed label Oct 27, 2023
@valentindavid
Copy link
Contributor Author

valentindavid commented Oct 27, 2023

!13278 with canonical/secboot#266

@valentindavid valentindavid added the Run nested The PR also runs tests inluded in nested suite label Oct 27, 2023
@valentindavid valentindavid reopened this Oct 27, 2023
@codecov-commenter
Copy link

codecov-commenter commented Oct 27, 2023

Codecov Report

Attention: 16 lines in your changes are missing coverage. Please review.

Comparison is base (cdbd316) 78.96% compared to head (6535984) 78.86%.
Report is 55 commits behind head on master.

Files Patch % Lines
secboot/secboot_hooks.go 52.38% 8 Missing and 2 partials ⚠️
secboot/secboot_sb.go 40.00% 2 Missing and 1 partial ⚠️
secboot/secboot_tpm.go 91.17% 2 Missing and 1 partial ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##           master   #13339      +/-   ##
==========================================
- Coverage   78.96%   78.86%   -0.10%     
==========================================
  Files        1028     1030       +2     
  Lines      129762   130548     +786     
==========================================
+ Hits       102462   102958     +496     
- Misses      20911    21177     +266     
- Partials     6389     6413      +24     
Flag Coverage Δ
unittests 78.86% <73.77%> (-0.10%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@valentindavid valentindavid force-pushed the valentindavid/move-to-latest-secboot-with-snakeoil branch 6 times, most recently from 17764c1 to 73a3ca3 Compare November 27, 2023 13:38
@valentindavid valentindavid force-pushed the valentindavid/move-to-latest-secboot-with-snakeoil branch from 73a3ca3 to 9e4325c Compare November 29, 2023 12:27
@valentindavid valentindavid reopened this Nov 30, 2023
@valentindavid valentindavid reopened this Dec 1, 2023
@valentindavid valentindavid force-pushed the valentindavid/move-to-latest-secboot-with-snakeoil branch from 9e4325c to ce639b4 Compare December 1, 2023 12:35
@valentindavid
Copy link
Contributor Author

tests/nested/core/core20-gadget-reseal started to fail with:

2023-12-05T17:20:27.3929553Z [   91.522913] snapd[1922]: 2023/12/05 17:10:31.229509 logger.go:93: DEBUG: 2023-12-05T17:10:31Z ERROR cannot set next boot: cannot reseal the fallback encryption keys: cannot increment counter: TPM returned a warning whilst executing command TPM_CC_StartAuthSession: TPM_RC_SESSION_MEMORY (out of memory for session contexts)

@valentindavid valentindavid force-pushed the valentindavid/move-to-latest-secboot-with-snakeoil branch 2 times, most recently from fa2ae65 to a984a37 Compare December 7, 2023 10:34
@valentindavid valentindavid force-pushed the valentindavid/move-to-latest-secboot-with-snakeoil branch 3 times, most recently from dc5464a to 8bc7657 Compare January 10, 2024 13:32
@valentindavid valentindavid changed the title go.mod: move to latest secboot #13278 with snakeoil go.mod: move to latest secboot Jan 10, 2024
@valentindavid valentindavid marked this pull request as ready for review January 10, 2024 13:35
Copy link
Member

@alfonsosanchezbeato alfonsosanchezbeato left a comment

Choose a reason for hiding this comment

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

I made a pass. An additional change that I wonder if is needed is that in the past parts of secboot were imported in snapd:
a466265
e45a076
and maybe we can use directly secboot now. But I don't know about the reason for importing code this way, maybe @pedronis has some context. Anyway, that should be possible a follow-up.

tests/nested/manual/uc-update-assets-secure/task.yaml Outdated Show resolved Hide resolved
tests/nested/manual/uc-update-assets-secure/task.yaml Outdated Show resolved Hide resolved
tests/nested/manual/uc-update-assets-secure/task.yaml Outdated Show resolved Hide resolved
tests/nested/core/core20-kernel-failover/task.yaml Outdated Show resolved Hide resolved
go.mod Outdated Show resolved Hide resolved
secboot/encrypt_sb.go Outdated Show resolved Hide resolved
secboot/secboot_hooks.go Outdated Show resolved Hide resolved
secboot/secboot_sb.go Outdated Show resolved Hide resolved
@valentindavid
Copy link
Contributor Author

I made a pass. An additional change that I wonder if is needed is that in the past parts of secboot were imported in snapd:
a466265
e45a076
and maybe we can use directly secboot now. But I don't know about the reason for importing code this way, maybe @pedronis has some context. Anyway, that should be possible a follow-up.

I had a quick look if I could build without it. It seems that we need this code to enroll recovery keys from snap-fde-keymgr. The functions it calls are in "internal" part of secboot and cannot be used directly.

@valentindavid
Copy link
Contributor Author

I have added the Block label, because we should not merge it yet. The new key format has to be merged together. But it should still be reviewed.

Copy link
Member

@alfonsosanchezbeato alfonsosanchezbeato left a comment

Choose a reason for hiding this comment

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

Thanks for the changes, I'm not approving yet as you mentioned that other things need to land first.

Copy link
Collaborator

@pedronis pedronis left a comment

Choose a reason for hiding this comment

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

lgtm, one question, did Chris have a look a the changes?

Comment on lines -78 to -83
var handle []byte
if keySetup.Handle == nil {
// this will reach fde-reveal-key as null but should be ok
handle = []byte("null")
} else {
handle = *keySetup.Handle
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

why we don't need this bit of code/manipulation anymore?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have no idea. I believe that was done by Michael. I suppose we can revert this one if it breaks customers' fde-reveal-key.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I suppose, if "fde-setup" does not set a handle. It probably does not care about the handle in the corresponding "fde-reveal-key".

@pedronis pedronis self-assigned this Jan 18, 2024
@valentindavid
Copy link
Contributor Author

lgtm, one question, did Chris have a look a the changes?

I do not know if he looked through it.

Copy link
Member

@alfonsosanchezbeato alfonsosanchezbeato left a comment

Choose a reason for hiding this comment

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

LGTM

@valentindavid valentindavid force-pushed the valentindavid/move-to-latest-secboot-with-snakeoil branch from 6535984 to 734eaf7 Compare March 19, 2024 12:03
@valentindavid valentindavid force-pushed the valentindavid/move-to-latest-secboot-with-snakeoil branch from 734eaf7 to 7b339f9 Compare May 7, 2024 12:44
@valentindavid valentindavid force-pushed the valentindavid/move-to-latest-secboot-with-snakeoil branch from 7b339f9 to bc9b81a Compare May 7, 2024 12:50
@valentindavid
Copy link
Contributor Author

Everything is in #13951. Closing this one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
⛔ Blocked Run Nested -auto- Label automatically added in case nested tests need to be executed Run nested The PR also runs tests inluded in nested suite
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants