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.

Sign up for GitHub

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

Jump to bottom

Alternative signing system #1878

Merged
merged 75 commits into from Apr 4, 2020
Merged

Alternative signing system #1878

merged 75 commits into from Apr 4, 2020

Conversation

d4s
Copy link
Member

@d4s d4s commented Jun 25, 2019 •
edited

Proposed the implementation of alternative signing system (as followup of #1233)

Approach is pretty simple: have a common interface for signing and verification, with implementation of particular sign/verify in separate modules.

  • could co-exists with current GPG implementation
  • allows to store and use multiple signature types in detached metadata in the same time
  • --with-libsodium option is needed to build with support of ed25515 signing engine

Current status:

  • added the very first version of signing interface
  • added "dummy" signing engine
  • added "ed25519" signing engine
  • added new builtin ostree sign (inspired by "ostree gpg-sign") allowing to sign and verify commits
  • able to sign/verify commits
  • able to sign/verify summary file
  • added tests for commits and summary signing/verification
  • added tests for pulling verification

not implemented:

  • have no support of printing the status on the screen (yet)
  • probably not integrated into all places there it should be used (need hints here)

New configuration keys

For repository and remotes:

  • sign-verify -- global and per-remote to trigger signature verification of updates
  • verification-key -- per-remote -- for ed25519: base64 encoded public key to use for verification
  • verification-file -- per-remote -- for ed25519: file with the list of base64 public keys to use for verification

Dummy engine

Accept any ASCII string as public/secret key. Used mostly for testing the signing interface itself. Support only single public key for verification.

Ed25519

Added "well-known" system places for ed25519 public keys -- expected 1 base64 key per line:

  • files
    • /etc/ostree/trusted.ed25519
    • DATADIR + /ostree/trusted.ed25519
  • and directories:
    • /etc/ostree/trusted.ed25519.d
    • DATADIR + /ostree/trusted.ed25519.d

The same is for revoked keys:

  • /etc/ostree/revoked.ed25519
  • /etc/ostree/revoked.ed25519.d
  • DATADIR + /ostree/revoked.ed25519
  • DATADIR + /ostree/rvokeded.ed25519.d

Current logic for verification during the commits/summary file pulling:

  • use verification-key if it exists in configuration
  • use verification-file if it exists in configuration
  • check public keys from well-known system places

@papr-bot
Copy link

Can one of the admins verify this patch?

@rh-atomic-bot
Copy link

Can one of the admins verify this patch?
I understand the following commands:

  • bot, add author to whitelist
  • bot, test pull request
  • bot, test pull request once

@d4s d4s mentioned this pull request Jun 25, 2019
Closed
@jlebon
Copy link
Member

jlebon commented Jun 26, 2019

bot, add author to whitelist

@cgwalters
Copy link
Member

cgwalters commented Jun 29, 2019 •
edited

Only skimmed this so far, I think it is looking reasonable.

Let's split off the ability to disable gpgme as a separate PR first?

Did you consider my suggestion to support a "plugin" mechanism that's just fork/exec of an external process? Like /usr/lib/ostree/signing.d/ or so, we iterate over all of them and we have a few well-known return codes like 0=success, 1=error, 77=signature type not recognized, 2=signed but failed to verify - It's basically what gpgme is doing, just with a lot of additional API for setup, parsing error messages etc.

The advantage is simplicity and libostree isn't linked against any additional libraries. The downside is less integration.

But to be clear I'm fine with this approach as well.

@d4s
Copy link
Member Author

d4s commented Jul 1, 2019

Thanks for reviewing!

Let's split off the ability to disable gpgme as a separate PR first?

Ok. May be a bit later -- going to vacation, so it would be delayed.
Sorry about that, but I really wanted to know first if the current approach fits for ostree, so will continue after my returning.

Did you consider my suggestion to support a "plugin" mechanism that's just fork/exec of an external process? Like /usr/lib/ostree/signing.d/ or so, we iterate over all of them and we have a few well-known return codes like 0=success, 1=error, 77=signature type not recognized, 2=signed but failed to verify - It's basically what gpgme is doing, just with a lot of additional API for setup, parsing error messages etc.

Yes, I thought about it, and it was a fallback variant tbh ;) Since you think that current approach is reasonable enough -- it wouldn't be a hard task to add a module for implementing that approach as well if needed.

But to be clear I'm fine with this approach as well.

Thanks! glad to know that. Will proceed with cleanup first.

@d4s d4s mentioned this pull request Jul 18, 2019
Closed
@d4s
Copy link
Member Author

d4s commented Jul 18, 2019

Added the first step in PR #1889 -- ability to build libostree without GPGME.

@rh-atomic-bot
Copy link

☔ The latest upstream changes (presumably cf7fc0e) made this pull request unmergeable. Please resolve the merge conflicts.

Thread974
Thread974 reviewed Aug 2, 2019
View reviewed changes
src/libostree/ostree-repo-pull.c Outdated Show resolved Hide resolved
src/libostree/ostree-repo-pull.c Outdated Show resolved Hide resolved
src/libostree/ostree-sign.c Outdated Show resolved Hide resolved
@rh-atomic-bot
Copy link

☔ The latest upstream changes (presumably 71e1e9d) made this pull request unmergeable. Please resolve the merge conflicts.

@d4s d4s force-pushed the wip/d4s/no_gpg branch 3 times, most recently from bdf4b08 to d3cae6e Compare August 27, 2019 08:03
Thread974
Thread974 suggested changes Aug 29, 2019
View reviewed changes
Makefile-libostree.am Show resolved Hide resolved
Makefile-libostree.am Show resolved Hide resolved
Makefile-ostree.am Show resolved Hide resolved
src/libostree/ostree-repo-pull.c Outdated Show resolved Hide resolved
src/libostree/ostree-repo-pull.c Outdated Show resolved Hide resolved
src/libostree/ostree-sign-ed25519.c Outdated Show resolved Hide resolved
src/libostree/ostree-sign-ed25519.c Outdated Show resolved Hide resolved
src/libostree/ostree-sign-ed25519.c Show resolved Hide resolved
src/libostree/ostree-sign-ed25519.c Outdated Show resolved Hide resolved
src/libostree/ostree-sign-ed25519.c Outdated Show resolved Hide resolved
@rh-atomic-bot
Copy link

☔ The latest upstream changes (presumably bdbce9d) made this pull request unmergeable. Please resolve the merge conflicts.

@d4s d4s force-pushed the wip/d4s/no_gpg branch 2 times, most recently from 9442f5e to 6c8f51d Compare October 8, 2019 09:39
d4s and others added 16 commits March 25, 2020 15:23
@d4s
lib/sign: make ed25519 engine non-public
b4050b4 
Remove unneeded public declaration for ed25519 signing engine.

Signed-off-by: Denis Pynkin <denis.pynkin@collabora.com>
@d4s
lib/sign: better error handling of ed25519 initialization
dd27461 
Add more precise error handling for ed25519 initialization.
Check the initialization status at the beginning of every public
function provided by ed25519 engine.

Signed-off-by: Denis Pynkin <denis.pynkin@collabora.com>
@d4s
lib/repo-pull: return error from signing engine
194ab36 
Return the collected errors from signing engines in case if verification
failed for the commit.

Signed-off-by: Denis Pynkin <denis.pynkin@collabora.com>
@d4s
lib/repo-pull: return errors from signature engines
59b9e64 
Improve error handling for signatures checks -- passthrough real
reasons from signature engines instead of using common messages.

Signed-off-by: Denis Pynkin <denis.pynkin@collabora.com>
@cgwalters @d4s
build-sys: Print libsodium status at end of configure
fbc5927 
Like we do with other features.
@cgwalters @d4s
sign-ed25519: Convert some functions to new style
2a0edcc 
The "new style" code generally avoids `goto err` because it conflicts
with `__attribute__((cleanup))`.  This fixes a compiler warning.
@cgwalters @d4s
sign-dummy: Convert to current code style
e2bd2ab 
This keeps the code style consistent.
@cgwalters @d4s
signing: Remove g_debug(__FUNCTION__)
9d02199 
This type of thing is better done via `gdb` and/or userspace
tracing (systemtap/bpftrace etc.)
@d4s
tests/sign: added check with file and single key on pull
09d5b47 
Additional test of signatures check behavior during the pull
with keys file containing wrong signatures and correct verification
key. Both are set as a part of remote's configuration.

Signed-off-by: Denis Pynkin <denis.pynkin@collabora.com>
@d4s
sign-ed25519: Convert functions to new style
aaf73f6 
The "new style" code generally avoids `goto err` because it conflicts
with `__attribute__((cleanup))`.  This fixes a compiler warning.

Signed-off-by: Denis Pynkin <denis.pynkin@collabora.com>
@d4s
sign-dummy: optimize ostree_sign_dummy_data_verify
1e3bdef 
Return TRUE as soon as any signature verified.

Signed-off-by: Denis Pynkin <denis.pynkin@collabora.com>
@d4s
lib/sign: convert ostree_sign_summary to new style
e16faa5 
The "new style" code generally avoids `goto err` because it conflicts
with `__attribute__((cleanup))`.  This fixes a compiler warning.

Signed-off-by: Denis Pynkin <denis.pynkin@collabora.com>
@d4s
tests/sign: check pull failure with invalid remote options
5a39281 
Pull should to fail if no known signature available in remote's
configuration or well-known places.

Signed-off-by: Denis Pynkin <denis.pynkin@collabora.com>
@d4s
lib/sign: return false for non-implemented functions
584ad40 
Do not mask implementation anymore since we have a working
engines integrated with pulling mechanism.

Signed-off-by: Denis Pynkin <denis.pynkin@collabora.com>
@d4s
sign-pull: improve error handling
cce3864 
Use glnx_* functions in signature related pull code for clear
error handling.

Signed-off-by: Denis Pynkin <denis.pynkin@collabora.com>
@d4s
ostree-repo: improve error handling
e2c6016 
Correctly return "error" from `ostree_repo_sign_commit()`
in case if GPG is not enabled.

Use glnx_* functions in signature related pull code for clear
error handling if GPG isn't enabled.

Signed-off-by: Denis Pynkin <denis.pynkin@collabora.com>
@d4s
Copy link
Member Author

d4s commented Mar 29, 2020

hmmm... @cgwalters, just a reminder about this PR ;)

@cgwalters
Copy link
Member

cgwalters commented Apr 4, 2020 •
edited

/test sanity

OK at this point I think we can commit to getting this in the next release, there will likely be followup work around this, but we can do it in master. Thanks for all of the work on this! And sorry about being slow on getting it in.

/lgtm

@openshift-ci-robot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cgwalters, d4s

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@cgwalters
Copy link
Member

That's weird, I don't know what's going on with the do-not-merge/invalid-owners-file label.

@cgwalters
Copy link
Member

/refresh

@cgwalters
Copy link
Member

/refresh

@openshift-merge-robot openshift-merge-robot merged commit a16fe86 into ostreedev:master Apr 4, 2020
@cgwalters
Copy link
Member

@d4s I submitted a followup PR here #2058 - mind reviewing?

@d4s d4s mentioned this pull request Apr 7, 2020
Merged
@cgwalters cgwalters mentioned this pull request May 26, 2020
Merged
@a-wai a-wai mentioned this pull request Jan 29, 2021
Open
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cgwalters cgwalters cgwalters left review comments

@fdanis-oss fdanis-oss fdanis-oss left review comments

@Thread974 Thread974 Thread974 requested changes

Assignees

@cgwalters cgwalters

Labels
approved lgtm
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet