WIP: Add containers_image_sequoia build tag to do simple signing verification using Sequoia-PGP, and add a signature/simplesequoia implementation #2876
Conversation
This adds a new OpenPGP signing mechanism based Sequoia-PGP[1]. As Sequoia-PGP is written in Rust and doesn't provide a stable C FFI, this integration includes a minimal shared library as a "point solution". To build, first follow the instruction in signature/sequoia/README.md and install `libimage_sequoia.so*` into the library path, and then build with the following command from the top-level directory: $ make BUILDTAGS="btrfs_noversion containers_image_sequoia" Note also that, for testing on Fedora, crypto-policies settings might need to be modified to explictly allow SHA-1 and 1024 bit RSA, as the testing keys in signature/fixtures are using those legacy algorithms. 1. https://sequoia-pgp.org/ Signed-off-by: Daiki Ueno <dueno@redhat.com>
8c6899b to
97955fb
Compare
| return Ok(()); | ||
| } | ||
| Err(verification_error) => { | ||
| signature_errors.push(verification_error.to_string()); |
There was a problem hiding this comment.
Just a matter of taste, but instead of storing errors as a string, I would keep the error types and let the caller format them:
diff --git a/signature/internal/sequoia/rust/src/signature.rs b/signature/internal/sequoia/rust/src/signature.rs
index 014d4f5f..d0ba6cd3 100644
--- a/signature/internal/sequoia/rust/src/signature.rs
+++ b/signature/internal/sequoia/rust/src/signature.rs
@@ -169,6 +169,7 @@ impl<'a> SequoiaMechanism<'a> {
let h = Helper {
certstore: self.certstore.clone(),
signer: Default::default(),
+ errors: Default::default(),
};
let mut v = VerifierBuilder::from_bytes(signature)?.with_policy(&self.policy, None, h)?;
@@ -190,6 +191,7 @@ impl<'a> SequoiaMechanism<'a> {
struct Helper<'a> {
certstore: Arc<sequoia_cert_store::CertStore<'a>>,
signer: Option<openpgp::Cert>,
+ errors: Vec<openpgp::Error>,
}
impl<'a> VerificationHelper for Helper<'a> {
@@ -205,7 +207,6 @@ impl<'a> VerificationHelper for Helper<'a> {
}
fn check(&mut self, structure: MessageStructure) -> openpgp::Result<()> {
- let mut signature_errors: Vec<String> = Vec::new();
for layer in structure {
match layer {
MessageLayer::Compression { algo } => log::info!("Compressed using {}", algo),
@@ -219,7 +220,7 @@ impl<'a> VerificationHelper for Helper<'a> {
log::info!("Encrypted using {}", sym_algo);
}
}
- MessageLayer::SignatureGroup { ref results } => {
+ MessageLayer::SignatureGroup { results } => {
for result in results {
match result {
Ok(good_checksum) => {
@@ -227,19 +228,14 @@ impl<'a> VerificationHelper for Helper<'a> {
return Ok(());
}
Err(verification_error) => {
- signature_errors.push(verification_error.to_string());
+ self.errors.push(openpgp::Error::from(verification_error));
}
}
}
}
}
}
- let err = match signature_errors.len() {
- 0 => anyhow::anyhow!("No valid signature"),
- 1 => anyhow::anyhow!("{}", &signature_errors[0]),
- _ => anyhow::anyhow!("Multiple signature errors: [{}]", signature_errors.join(", ")),
- };
- Err(err)
+ Err(anyhow::anyhow!("No valid signature"))
}
}I also wonder if we might want to record all signers if there are multiple signatures.
There was a problem hiding this comment.
I would keep the error types and let the caller format them:
Thanks, that might very well be cleaner. It would require a somewhat tight coupling with the caller — the caller would specifically need to know that it should ignore the returned “no valid signature” error.
I also wonder if we might want to record all signers if there are multiple signatures.
For myself, I think I need to study / understand what is the precise semantics of MessageStructure, whether it is possible to have multiple signatures, and what does that mean.
… and whether it is relevant to designing a stable C API, which is the ~most urgent work right now.
There was a problem hiding this comment.
I would keep the error types and let the caller format them:
Thanks, that might very well be cleaner. It would require a somewhat tight coupling with the caller — the caller would specifically need to know that it should ignore the returned “no valid signature” error.
Actually that's the intention behind this suggestion: if the errors are converted into a string at the callee, useful information that could be retrieved through VerificationError may be lost, or it would require parsing of the error strings later.
I also wonder if we might want to record all signers if there are multiple signatures.
For myself, I think I need to study / understand what is the precise semantics of
MessageStructure, whether it is possible to have multiple signatures, and what does that mean.… and whether it is relevant to designing a stable C API, which is the ~most urgent work right now.
MessageStructure can certainly have multiple signatures, though the current c/image API seems to only expect a single signature to be valid. For future extension with PQ/T multiple signatures, it might make sense to provide a control on which signature has to be valid and which can be ignored.
As for the C API, GPGME provides a similar abstraction with gpgme_verify_result_t, which also supports multiple signatures.
23e7645 to
7e16175
Compare
efcf53d to
461beea
Compare
signature/simplesequoia/signer.go
Outdated
| func WithPassphrase(passphrase string) Option { | ||
| return func(s *simpleSequoiaSigner) error { | ||
| // The gpgme implementation can’t use passphrase with \n; reject it here for consistent behavior. | ||
| // FIXME: We don’t need it in this API at all, but the "\n" check exists in the current call stack. That should go away. |
There was a problem hiding this comment.
Branch sequoia-refactor-SignDockerManifest, but that can wait.
| return nil, fmt.Errorf("Signing not supported: %w", err) | ||
| } | ||
|
|
||
| // Ideally, we should look up (and unlock?) the key at this point already. FIXME: is that possible? Anyway, low-priority. |
There was a problem hiding this comment.
Yes, if needed we could reorganize the Rust API to allow that.
83975b0 to
e19a9d0
Compare
| sig := C.go_sequoia_sign( | ||
| m.mechanism, | ||
| cKeyIdentity, | ||
| cPassphrase, | ||
| (*C.uchar)(unsafe.Pointer(unsafe.SliceData(input))), | ||
| C.size_t(len(input)), | ||
| &cerr, | ||
| ) | ||
| if sig == nil { | ||
| defer C.go_sequoia_error_free(cerr) | ||
| return nil, errors.New(C.GoString(cerr.message)) | ||
| } | ||
| defer C.go_sequoia_signature_free(sig) | ||
| var size C.size_t | ||
| cData := C.go_sequoia_signature_get_data(sig, &size) | ||
| if size > C.size_t(C.INT_MAX) { | ||
| return nil, errors.New("overflow") | ||
| } | ||
| return C.GoBytes(unsafe.Pointer(cData), C.int(size)), nil |
There was a problem hiding this comment.
I mention it because we have been burned by this many times before and I haven't looked into the sequoia code to know for sure but is the sequoia library fully thread safe. And by that I mean is it using any thread local memory or other things that depend on the current thread? Because the go runtime can schedule us around in any thread at any time between these C calls which means go_sequoia_sign() and go_sequoia_signature_get_data() may get called from different threads so the underlying library must be able to handle this correctly
i.e. a simple example on how things can go wrong easily:
coreos/go-systemd@d1df97f
There was a problem hiding this comment.
On the Go side, this PR now uses a sync.Once to load the library. And afterwards, everything is supposed to go through a Go *SigningMechanism / Rust SequoiaMechanism, where serialization of operations on a single mechanism is up to the caller.
I have no idea whether Sequoia is internally consistently thread-safe; I do see some uses of OnceLock on static variables, so it is at least not naively unaware.
There was a problem hiding this comment.
serialization of operations on a single mechanism is up to the caller.
My concern was not so much about serialization but more about the use of stuff like thread local storage or other per thread things like a namespace for example. Between each C.somefunc call the go runtime can schedule us the code onto another thread. But because that is super unlikely normal testing will not uncover this most likely. It could end up being a very hard to find race condition later on. See the linked commit where the use of dlerror() causes some bugs because it stores the error in thread local memory.
I am not saying there is a problem here but just that we should check. I am sure @ueno or the other sequoia folks would know this
f7dbbbe to
7871386
Compare
47d405a to
a5bc6cc
Compare
a1bf189 to
9bb1701
Compare
Encapsulate the wanted/recorded tuples a bit more, to prepare for changes to the signatureAcceptanceRules type. Should not change (test) behavior. Signed-off-by: Miloslav Trmač <mitr@redhat.com>
…ryKeyFingerprints Move more code into the centralized function, to allow us to change the implementation in the future. Signed-off-by: Miloslav Trmač <mitr@redhat.com>
Document how the key identities from NewEphemeralGPGSigningMechanism and SigningMechanism should relate. For the GPGME implementation where the relationship isn't as desired, introduce signingMechanismWithVerificationIdentityLookup . For the openpgp implementation, just fix the returned value to match new expecations. Currently, the simple signing verifyAndExtractSignature rejects subkey signatures for both mechanism implementations, so this does not change behavior for any previously-accepted signatures (but it might affect theoretical external direct callers of SigningMechanism). We will actually use this in the next commit. WARNING: Adding support for subkeys makes subkey revocation relevant. Revocation is always messy - to support long-term signature validity (which is very relevant for container image signing), smooth handling of revocation might require trusted timestamps (not natively supported in OpenPGP). In practice, this library (partially out of necessity) lets the underlying OpenPGP implementations impose their own policy, and that handling DIFFERS between various SigningMechanism implementations: it seems that GnuPG and refuses to use a revoked subkey regardless of revocation time, while Sequoia-PGP decides based on the revocation reason whether the revocation applies to all signatures or only to signatures (supposedly!!) made after the revocation happened. It is STRONGLY recommended to avoid all of this complexity, and to plan for workflows where subkey revocation never happens. Just revoke and reissue the whole key - either way you'll need to distribute an updated key to all consumers. Signed-off-by: Miloslav Trmač <mitr@redhat.com>
This does not change behavior for mechanisms which already always return the primary key fingerprint (after a recent change, openpgp); for the others, add support for signingMechanismWithVerificationIdentityLookup . Signed-off-by: Miloslav Trmač <mitr@redhat.com>
…iners-image into signature-sequoia
Signed-off-by: Miloslav Trmač <mitr@redhat.com>
- Use the correct library name - Allow specifying a library path at compile time Example usage: > make BUILDTAGS="btrfs_noversion containers_image_sequoia" SEQUOIA_SONAME_DIR=$(pwd)/signature/internal/sequoia/rust/target/release Signed-off-by: Miloslav Trmač <mitr@redhat.com>
... to match the recent more specific subkey validation semantics. Should not change behavior. Signed-off-by: Miloslav Trmač <mitr@redhat.com>
Signed-off-by: Miloslav Trmač <mitr@redhat.com>
Signed-off-by: Miloslav Trmač <mitr@redhat.com>
Signed-off-by: Miloslav Trmač <mitr@redhat.com>
Should not change test behavior, merely to be more similar to the rest of the codebase. Signed-off-by: Miloslav Trmač <mitr@redhat.com>
…Mechanism Signed-off-by: Miloslav Trmač <mitr@redhat.com>
It can be handled in callers - and it's better to do based on .Init() outcomes. Signed-off-by: Miloslav Trmač <mitr@redhat.com>
- Allow using c/image without loading sequoia if GO_SEQUOIA_ENABLE_DLOPEN is used, and nothing uses the Sequoia code at runtime. - Allow adding ~independent users of signature/internal/sequoia, without having to coordinate to call Init at least once. We will add signature/simplesequoia to benefit from this. Signed-off-by: Miloslav Trmač <mitr@redhat.com>
We can't reasonably unit-test the $HOME variant; tested manually. Signed-off-by: Miloslav Trmač <mitr@redhat.com>
By default, the Rust logging output is silently discarded. Signed-off-by: Miloslav Trmač <mitr@redhat.com>
Signed-off-by: Miloslav Trmač <mitr@redhat.com>
Add missing tests, or add comments where that's not practical. Should not change behavior. Signed-off-by: Miloslav Trmač <mitr@redhat.com>
…ner API Signed-off-by: Miloslav Trmač <mitr@redhat.com>
Should not change behavior. Signed-off-by: Miloslav Trmač <mitr@redhat.com>
For now only local tests, we should also do this for test_skopeo, but that ~requires a corresponding Skopeo update. WIP: We want to use distribution packages instead of compiling Rust code ourselves. Signed-off-by: Miloslav Trmač <mitr@redhat.com>
Requires an unmerged Skopeo PR. Signed-off-by: Miloslav Trmač <mitr@redhat.com>
... to save on irrelevant cloud costs and noise, for now. Signed-off-by: Miloslav Trmač <mitr@redhat.com>
9bb1701 to
55d1eaf
Compare
- Set VM_IMAGE_NAME on the currently-deleted tests as well DO NOT MERGE: Depends on UNMERGED containers/automation_images#411 . Signed-off-by: Miloslav Trmač <mitr@redhat.com>
Sharing this to possibly help collaboration; unlikely to be useful for any real-world use, anything finished will probably be split into separate PRs.
Based on #2569 , many thanks to @ueno .
DO NOT MERGE: Includes UNMERGED #2905 .