verify: init #311

jleightcap · 2023-11-16T18:38:09Z

Summary

Complementary half of #310, enable the verification of bundles:

$ cargo run --manifest-path tests/conformance/Cargo.toml -- sign-bundle --bundle bundle.txt.sigstore --identity-token $(sigstore-python get-identity-token) bundle.txt
# ...
Operation succeeded!
$ cargo run --manifest-path tests/conformance/Cargo.toml -- verify-bundle --bundle bundle.txt.sigstore --certificate-identity jack@leightcap.com --certificate-oidc-issuer https://github.com/login/oauth bundle.txt
# ...
Operation succeeded!

Release Note

Documentation

Add ability to sign artifacts à la sigstore-python, towards Bundle signing and verification. Follow-up from #305, and the prerequisite motivation to #311. Signed-off-by: Jack Leightcap <jack.leightcap@trailofbits.com> Signed-off-by: Andrew Pan <andrew.pan@trailofbits.com> Signed-off-by: Andrew Pan <3821575+tnytown@users.noreply.github.com> Signed-off-by: Jack Leightcap <30168080+jleightcap@users.noreply.github.com> Co-authored-by: Andrew Pan <a@tny.town> Co-authored-by: Andrew Pan <3821575+tnytown@users.noreply.github.com> Co-authored-by: Andrew Pan <andrew.pan@trailofbits.com> Co-authored-by: Flavio Castelli <flavio@castelli.me>

src/crypto/certificate.rs

jleightcap · 2024-01-05T22:33:44Z

src/crypto/certificate.rs

+    }
+
+    // A certificate that is its own issuer and signer is considered a root CA.
+    // TODO(jl): verify_directly_issued_by


Need to find a Rust equivalent of https://cryptography.io/en/latest/x509/reference/#cryptography.x509.Certificate.verify_directly_issued_by

jleightcap · 2024-01-05T22:34:47Z

src/verify/models.rs

+
+                if inclusion_proof.is_some() && !has_checkpoint {
+                    // TODO(tnytown): Act here.
+                    // NOTE(jl): in sigstore-python, this is a no-op that prints a warning log.


I'm not sure how best to flag a warning of invalid state in this case, AFAIK there isn't a logging framework to hook into given this is a library crate.

i think we depend on tracing, so we could potentially use that. a good follow-on task might be to go around the new bundle code and add traces + define a reasonable tracing span hierarchy.

src/verify/models.rs

tnytown

thanks for fixing this up! i have a few comments and clarifications :)

tnytown · 2024-01-05T23:00:50Z

src/crypto/verification.rs

+use rustls_pki_types::CertificateDer;
+use webpki::TrustAnchor;
+
+/// Machinery for Sigstore end entity certificate verification.
+struct CertificateVerificationContext<'a> {
+    pub trust_anchors: Vec<TrustAnchor<'a>>,
+    pub intermediate_certs: Vec<CertificateDer<'a>>,
+}


this is chaff from an older iteration of the API, we can probably remove it!

src/verify/models.rs

tnytown · 2024-01-05T23:08:19Z

src/verify/models.rs

+
+                if inclusion_proof.is_some() && !has_checkpoint {
+                    // TODO(tnytown): Act here.
+                    // NOTE(jl): in sigstore-python, this is a no-op that prints a warning log.


i think we depend on tracing, so we could potentially use that. a good follow-on task might be to go around the new bundle code and add traces + define a reasonable tracing span hierarchy.

tnytown · 2024-01-05T23:09:09Z

src/tuf/constants.rs

@@ -23,3 +23,4 @@ macro_rules! tuf_resource {
 }

 pub(crate) const SIGSTORE_ROOT: &[u8] = tuf_resource!("prod/root.json");
+pub(crate) const _SIGSTORE_TRUST_BUNDLE: &[u8] = tuf_resource!("prod/trusted_root.json");


we may want to investigate taking advantage of this in the tuf module

woodruffw · 2024-01-16T20:37:45Z

src/verify/verifier.rs

+        let (_, extended_key_usage_ext): (bool, ExtendedKeyUsage) = materials
+            .certificate
+            .tbs_certificate
+            .get()
+            .expect("Malformed certificate")
+            .expect("Malformed certificate");
+
+        if !extended_key_usage_ext.0.contains(&ID_KP_CODE_SIGNING) {
+            return Err(VerificationError::CertificateTypeError(
+                "Extended key usage does not contain `code signing`".into(),
+            ));
+        }


This is already covered by store.verify_cert_with_time above, so I think it can be removed outright.

woodruffw · 2024-01-16T20:38:24Z

src/verify/verifier.rs

+        // TODO(tnytown): How likely is a malformed certificate in this position? Do we want to
+        // account for it and create an error type as opposed to unwrapping?
+        let (_, key_usage_ext): (bool, KeyUsage) = materials
+            .certificate
+            .tbs_certificate
+            .get()
+            .expect("Malformed certificate")
+            .expect("Malformed certificate");
+
+        if !key_usage_ext.digital_signature() {
+            return Err(VerificationError::CertificateTypeError(
+                "Key usage is not of type `digital signature`".into(),
+            ));
+        }


This check should be unnecessary, since chain building (above) should assert that the leaf contains a keyUsage.digitalSignature.

woodruffw · 2024-01-23T22:33:14Z

src/verify/models.rs

+        let rekor_entry = if let Some(rekor_entry) = rekor_entry {
+            let cell = OnceCell::new();
+
+            // TODO(tnytown): Switch to setting if offline when Rekor fetching is implemented.
+            #[allow(clippy::unwrap_used)]
+            cell.set(rekor_entry).unwrap();
+
+            cell
+        } else {
+            Default::default()
+        };


I'm having a little trouble following this: why do we put LogEntry into a OnceCell, rather than just keeping it as Option<LogEntry> with a None variant when we're missing an entry?

no good reason, I think I originally used OnceCell to allow for interior mutability when we lazy-init the entry; I don't think that's ever done in practice (and we shouldn't need to cache the online entry anyways.)

woodruffw · 2024-01-23T22:37:05Z

src/bundle/mod.rs

+macro_rules! required {
+    ($($base:expr )? ; $first_attr:ident $( . $rest_attrs:ident)* $( , $else_err:expr)?) => {
+        $( $base . )? $first_attr.as_ref()
+            $(
+                .and_then(|v| v.$rest_attrs.as_ref())
+            )*
+        $( .ok_or($else_err) )?
+    }
+}
+pub(crate) use required;


IMO this macro's usages are pretty hard to read -- I think we'd possibly be better off unfolding these inline (or using more bindings to minimize the number of error sites), plus implementing TryInto or another conversion trait where appropriate.

this wouldn't be necessary with better protobuf codegen. we can put this PR on hold until we figure that out

Signed-off-by: Jack Leightcap <jack.leightcap@trailofbits.com>

jleightcap · 2024-01-31T15:55:28Z

@flavio I'll continue to flush out documentation and cleanup this afternoon, but this should be feature complete. let me know if there's anything I can do to help facilitate review here.

flavio · 2024-02-01T10:06:14Z

@jleightcap, I skimmed through the PR and I've the following questions:

There's at least a todo()! invocation. I think we cannot merge that code until the actual implementation is provided
What about all the TODO(s)? Should we address them before merging it?
Can you look into the failed checks please?
I'm probably saying something stupid, but... isn't this verification code redoing some of the checks done by sigstore::cosign? What are the plans for the new verify module?

jasperpatterson

I was playing around with these changes as I'm excited to see this functionality merged and noticed a couple little issues in my testing.

jasperpatterson · 2024-02-14T22:07:16Z

src/verify/models.rs

+            return None;
+        }
+
+        for chain_cert in chain_certs {


Should the chain_certs be added to the VerificationMaterials? It appears they are thrown away right now and chain verification does not currently work if intermediates are present.

IIRC, intermediates should never be included in materials produced against the public-good instance; if they are present, the spec says we should ignore them. Fulcio's intermediates, if any, are distributed via TUF. For non-PGI verifications, we would want to plumb chain_certs through.

jasperpatterson · 2024-02-14T22:14:07Z

src/verify/models.rs

+            log_index: required!(tlog_entry; log_index)?.parse().ok()?,
+            verification: Verification {
+                inclusion_proof: parsed_inclusion_proof,
+                signed_entry_timestamp: required!(; inclusion_promise.signed_entry_timestamp)?


I believe this is not required for v0.2+ bundles? https://github.com/sigstore/protobuf-specs/blob/aa56b9a4929364918688fc7eeec3724c9941ddf9/protos/sigstore_rekor.proto#L105

Correct! I think the newer revisions I've pushed up handle this correctly.

jasperpatterson · 2024-02-14T22:22:05Z

src/crypto/certificate.rs

+        Some((false, _)) => {
+            // BasicConstraints must be marked as critical, per RFC 5280 4.2.1.9.
+            return Err(SigstoreError::InvalidCertError(
+                "invalid X.509 certificate: non-critical BasicConstraints in CA".to_string(),


In is_root_ca, you're assuming an Ok result from this function, to determine whether something isn't a CA cert:

if !is_ca(certificate)? { return Ok(false); }

However this situation is valid for an end certificate, and not an error, meaning that current logic doesn't quite work:

https://www.rfc-editor.org/rfc/rfc5280#section-4.2.1.9

This extension MAY appear as a critical or non-critical extension in end entity certificates.

Good catch!

jasperpatterson · 2024-02-14T22:38:18Z

src/verify/models.rs

+        }
+
+        for chain_cert in chain_certs {
+            if is_root_ca(chain_cert).is_ok() {


I think you need to specifically check the bool here, not just for Ok.

Yeah, you're right. I don't know how I feel about the Result<bool> return type, it feels easy to misuse. Maybe it'd be better to switch to bool and collapse the error states into false.

I changed is_ca, is_leaf, and is_root_ca to return Result<(), _>.

Signed-off-by: Andrew Pan <andrew.pan@trailofbits.com>

tnytown

Programmatic note: we @trailofbits are out of time on sigstore-rs. If this is not reviewed and merged by the end of the month, we will have to drop this changeset. I've fixed up what I can this cycle, but we don't have capacity for too many further review cycles.

tnytown · 2024-04-10T00:55:17Z

src/crypto/verification_key.rs

+
+    /// Verify the signature provided has been actually generated by the given key
+    /// when signing the provided prehashed message.
+    pub fn verify_prehash(&self, signature: Signature, msg: &[u8]) -> Result<()> {


I've changed the visibility to pub(crate) for now.

tnytown · 2024-04-10T00:55:26Z

src/errors.rs

@@ -109,6 +109,9 @@ pub enum SigstoreError {
    #[error("Certificate pool error: {0}")]
    CertificatePoolError(String),

+    #[error("Certificate invalid: {0}")]
+    InvalidCertError(String),


tnytown · 2024-04-10T01:05:02Z

src/fulcio/models.rs

-    serde_json::from_slice(&buf).map_err(serde::de::Error::custom)
+    let buf: Vec<u8> = Base64::<Standard, Padded>::deserialize_as(de)?;
+
+    // The first two bytes indicate the signature and hash algorithms so let's skip those.


This is not a huge deal since we are getting this info from the trustroot CTFE keys. We could certainly assert on an expected value here if you'd prefer :)

tnytown · 2024-04-10T01:06:27Z

src/fulcio/models.rs

+
+    // The first two bytes indicate the signature and hash algorithms so let's skip those.
+    // The next two bytes indicate the size of the signature.
+    let signature_size = u16::from_be_bytes(buf[2..4].try_into().expect("unexpected length"));


IIRC usize::from_be_bytes accepts exactly 4 bytes

tnytown · 2024-04-10T01:11:41Z

src/lib.rs

@@ -285,8 +285,13 @@ pub mod rekor;
 pub mod tuf;

 // Don't export yet -- these types should only be useful internally.
+#[cfg(feature = "bundle")]


It was supposed to gate our sigstore_protobuf_specs dep, but apparently I didn't do so. I've updated it now :)

tnytown · 2024-04-10T18:56:32Z

src/tuf/mod.rs

-    pub fn prefetch(self) -> Result<Self> {
-        let _ = self.trusted_root()?;
-        Ok(self)
+    pub fn new(checkout_dir: Option<&Path>) -> Result<Self> {


Fair point, updated :)

tnytown · 2024-04-10T19:01:18Z

src/tuf/mod.rs

+        Ok(Self { trusted_root })
+    }
+
+    fn fetch_target<N>(


Noted, interesting food for thought :) I think we'd need to put some further thought into supporting those kinds of environments (a pure AsyncRead + AsyncWrite wouldn't work because we support multiple targets.)

tnytown · 2024-04-10T19:05:52Z

src/tuf/mod.rs

+        let local_path = checkout_dir.as_ref().map(|d| d.join(name.raw()));
+
+        // Try reading the target from disk cache.
+        let data = if let Some(Ok(local_data)) = local_path.as_ref().map(std::fs::read) {


Hm, we wouldn't be able to naively return the error here: the file potentially doesn't exist in the cache yet.

tnytown · 2024-04-10T19:06:04Z

src/tuf/mod.rs

+        // Write the up-to-date data back to the disk. This doesn't need to succeed, as we can
+        // always fetch the target again later.
+        if let Some(local_path) = local_path {
+            let _ = std::fs::write(local_path, &data);


tnytown · 2024-04-10T19:12:51Z

src/verify/policy.rs

+
+        // Parse raw string without DER encoding.
+        let val = std::str::from_utf8(ext.extn_value.as_bytes())
+            .expect("failed to parse constructed Extension!");


Nice catch! I didn't realize that this was user input.

Signed-off-by: Andrew Pan <andrew.pan@trailofbits.com>

tnytown · 2024-04-10T19:28:22Z

CC @flavio, this is ready for review :)

Signed-off-by: Andrew Pan <andrew.pan@trailofbits.com>

flavio

LGTM, I left some comments.

Aside from my comments, it would be great to see the other comments addressed. For TODO stuff/nodes the outcome of the conversation can be added into the source code as a comment

flavio · 2024-04-11T10:55:48Z

src/trust/sigstore/mod.rs

I'm happy about the changes, it's also great to see the TUF contents are now synchronized with the local checkout.

Can you please add some unit tests to cover that? The repository_helper file (removed by this PR) had some tests you might be able to reuse

71f5161

I switched to using the online TUF repo in tests and deleted the copy we had checked in.

flavio · 2024-04-11T14:11:07Z

src/verify/verifier.rs

+/// A synchronous Sigstore verifier.
+///
+/// Async callers must use [`AsyncVerifier`]. Async usage of [`Verifier`] will result in a deadlock.
+pub struct Verifier {


I like @tannaurus' suggestion

tests/conformance/conformance.rs

Signed-off-by: Andrew Pan <andrew.pan@trailofbits.com>

tnytown

I think I've addressed all previous comments at this point (I reacted when only action was taken and commented in all other cases); let me know if I've missed any. I don't have permission to dismiss conversations on this PR because I'm not the author.

CC @flavio

tnytown · 2024-04-11T23:09:22Z

src/verify/verifier.rs

+/// A synchronous Sigstore verifier.
+///
+/// Async callers must use [`AsyncVerifier`]. Async usage of [`Verifier`] will result in a deadlock.
+pub struct Verifier {


It is done: 1e73783

tests/conformance/conformance.rs

tnytown · 2024-04-11T23:11:40Z

src/trust/sigstore/mod.rs

71f5161

I switched to using the online TUF repo in tests and deleted the copy we had checked in.

flavio

I left a minor comment, I'm fine with this PR.

flavio · 2024-04-12T06:53:34Z

src/trust/sigstore/mod.rs

+        );
+    }
+
+    macro_rules! impl_test {


I understand why you're using this macro, but it makes the tests less readable. You can achieve the same result using the rstest crate, which is already a dev dependency.

Finally, you can use the tokio::test macro to decorate the unit test, which removes more boilerplate from the test.

I can do the changes afterwards, I understand this PR has been ongoing since a long time

flavio · 2024-04-12T06:58:17Z

@tannaurus: can you take one last look at the PR before I merge it?

Signed-off-by: Andrew Pan <andrew.pan@trailofbits.com>

flavio

I think we're finally ready to merge it. Sorry for the long time it took!

tannaurus · 2024-04-18T16:16:35Z

I'm so happy to see this merged! Thanks for all the work @tnytown and @jleightcap ! ❤️ 🚀

Sorry for my delayed response here. I was out of town for team offsite and then came down with a bug when I got back.

jleightcap force-pushed the jl/verify branch from bdb9d95 to 7cdc30a Compare November 16, 2023 18:55

jleightcap mentioned this pull request Nov 16, 2023

sign: init #310

Merged

woodruffw mentioned this pull request Nov 21, 2023

sign/verify: add bundle support #309

Closed

jleightcap force-pushed the jl/verify branch 2 times, most recently from 3246492 to b3dc1ea Compare December 6, 2023 18:36

jleightcap mentioned this pull request Dec 7, 2023

test: bundles + conformance suite #315

Draft

jleightcap force-pushed the jl/verify branch 5 times, most recently from dfecc13 to 5c1792a Compare January 5, 2024 22:23

jleightcap commented Jan 5, 2024

View reviewed changes

jleightcap marked this pull request as ready for review January 5, 2024 22:36

tnytown reviewed Jan 5, 2024

View reviewed changes

woodruffw suggested changes Jan 16, 2024

View reviewed changes

tnytown mentioned this pull request Jan 22, 2024

Signed Certificate Timestamp verification #326

Merged

jleightcap force-pushed the jl/verify branch from 5c1792a to 39e2163 Compare January 23, 2024 20:59

woodruffw reviewed Jan 23, 2024

View reviewed changes

jleightcap added 2 commits January 31, 2024 10:54

verify: init

69f64fa

Signed-off-by: Jack Leightcap <jack.leightcap@trailofbits.com>

pr feedback 2024-01-31

6af5c49

Signed-off-by: Jack Leightcap <jack.leightcap@trailofbits.com>

jleightcap force-pushed the jl/verify branch from 39e2163 to 6af5c49 Compare January 31, 2024 15:54

This was referenced Jan 31, 2024

Route away from experimental to 1.0 #274

Open

pb-rust: codegen via prost + prost-reflect sigstore/protobuf-specs#207

Merged

tnytown mentioned this pull request Feb 14, 2024

Support for inclusion/consistency proofs and checkpoint/STH verification. #283

Open

jasperpatterson reviewed Feb 14, 2024

View reviewed changes

This was referenced Apr 9, 2024

Regression: TUF contents are not saved to disk anymore #345

Closed

Do not expose the rustls_pki_types::CertificateDer object inside of public API #347

Open

tnytown force-pushed the jl/verify branch 3 times, most recently from 1748f68 to b16136d Compare April 10, 2024 00:25

Merge remote-tracking branch 'origin/main' into jl/verify

34985ad

Signed-off-by: Andrew Pan <andrew.pan@trailofbits.com>

tnytown force-pushed the jl/verify branch from b16136d to 34985ad Compare April 10, 2024 00:33

tnytown added 2 commits April 10, 2024 14:17

2024-04-10 pr feedback

caec81c

Signed-off-by: Andrew Pan <andrew.pan@trailofbits.com>

verify: publicize verify_digest

11970d5

Signed-off-by: Andrew Pan <andrew.pan@trailofbits.com>

tnytown reviewed Apr 10, 2024

View reviewed changes

cargo fmt

39ea179

Signed-off-by: Andrew Pan <andrew.pan@trailofbits.com>

tnytown added 3 commits April 10, 2024 18:21

adjust bundle, sign, verify feature flags

ecf8995

Signed-off-by: Andrew Pan <andrew.pan@trailofbits.com>

fixup cargo build on features bundle, wasm

3f150a2

Signed-off-by: Andrew Pan <andrew.pan@trailofbits.com>

verifier: fixup time range check

81ae539

Signed-off-by: Andrew Pan <andrew.pan@trailofbits.com>

flavio requested changes Apr 11, 2024

View reviewed changes

tnytown added 2 commits April 11, 2024 16:48

trust/sigstore: test caching behavior

71f5161

Signed-off-by: Andrew Pan <andrew.pan@trailofbits.com>

sign, verify: reparent to bundle, doc fixups

1e73783

Signed-off-by: Andrew Pan <andrew.pan@trailofbits.com>

tnytown force-pushed the jl/verify branch from d22c985 to 1e73783 Compare April 11, 2024 23:07

tnytown reviewed Apr 11, 2024

View reviewed changes

flavio reviewed Apr 12, 2024

View reviewed changes

This was referenced Apr 12, 2024

remove async from trustroot trait #350

Closed

fix: allow ManualTrustRoot to hold multiple Rekor keys #349

Closed

feat: allow to create static instances of some objects #348

Closed

trust/sigstore: rework tests

c018346

Signed-off-by: Andrew Pan <andrew.pan@trailofbits.com>

flavio approved these changes Apr 17, 2024

View reviewed changes

flavio merged commit d5279eb into sigstore:main Apr 17, 2024
6 checks passed

woodruffw deleted the jl/verify branch April 18, 2024 17:45

verify: init #311

verify: init #311

Conversation

jleightcap commented Nov 16, 2023 • edited Loading

Summary

Release Note

Documentation

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tnytown left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jleightcap commented Jan 31, 2024

flavio commented Feb 1, 2024

jasperpatterson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tnytown Feb 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasperpatterson Feb 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tnytown left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tnytown commented Apr 10, 2024

flavio left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tnytown Apr 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tnytown left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tnytown Apr 11, 2024 • edited Loading

Choose a reason for hiding this comment

flavio left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

flavio commented Apr 12, 2024

flavio left a comment

Choose a reason for hiding this comment

tannaurus commented Apr 18, 2024

jleightcap commented Nov 16, 2023 •

edited

Loading

tnytown Feb 15, 2024 •

edited

Loading

jasperpatterson Feb 14, 2024 •

edited

Loading

tnytown Apr 11, 2024 •

edited

Loading

tnytown Apr 11, 2024 •

edited

Loading