Skip to content

Comments

Fix ekcert checking#21

Merged
kfox1111 merged 1 commit intospiffe:masterfrom
arianvp:fix-ekcert-checking
Feb 11, 2026
Merged

Fix ekcert checking#21
kfox1111 merged 1 commit intospiffe:masterfrom
arianvp:fix-ekcert-checking

Conversation

@arianvp
Copy link

@arianvp arianvp commented Aug 23, 2025

Fixes #19

@arianvp
Copy link
Author

arianvp commented Oct 9, 2025

@kfox1111 care to have a look? Without this golang fails to validate spec-compliant EKCerts

Copy link
Collaborator

@kfox1111 kfox1111 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 working on this. Sorry for the delay. Some questions inline

Comment on lines 194 to 198
oidTcgKpEKCertificate := asn1.ObjectIdentifier{2, 23, 133, 8, 1}
for _, id := range ek.Certificate.UnknownExtKeyUsage {
if id.Equal(oidTcgKpEKCertificate) {
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Dead code?

Copy link
Author

Choose a reason for hiding this comment

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

oh no I think we should throw an error here if it's not this key usage. Let me add that.

Copy link
Author

Choose a reason for hiding this comment

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

To solve this correctly in a way that is forwards and backwards compatible we need golang/go#75325

Otherwise, if golang ever starts accepting this OID in the standard library, this code will break.

Luckily this just landed in Go 1.26

How aggressive is SPIRE with adopting new go versions? If we are fine with requiring Go 1.26,I can make the check. Otherwise I suggest we remove this code completely and file an issue to improve this later

Copy link
Author

Choose a reason for hiding this comment

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

go 1.26 lands in February btw

Copy link
Author

Choose a reason for hiding this comment

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

after further reading the spec; the field is not critical and also not mandatory. it's marked as Extended Key Usage ExtKeyUsageSyntax tcg-kp-EKCertificate MAY non-critical

So I think I'm going to leave it out for now. As there might be EK Certs in the wild that do not have the field.

https://trustedcomputinggroup.org/wp-content/uploads/TCG-EK-Credential-Profile-for-TPM-Family-2.0-Level-0-Version-2.6_pub.pdf

Copy link
Author

Choose a reason for hiding this comment

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

Oh look at this: Merged a few days ago: google/go-attestation#471 🥳

@arianvp
Copy link
Author

arianvp commented Nov 11, 2025

Hmm any idea why CI is not triggering on my PR?

@kfox1111
Copy link
Collaborator

Hmm any idea why CI is not triggering on my PR?

hmm... offhand, no. It looks to me like it should work... will try and dig in soon.

@torfjor
Copy link

torfjor commented Jan 22, 2026

Friendly bump 👋. Would be nice to have this upstream!

@kfox1111
Copy link
Collaborator

Thanks for the reminder. will have a look

@kfox1111
Copy link
Collaborator

Looks like the only thing outstanding is a fix for the deadcode/exception handling.

@arianvp
Copy link
Author

arianvp commented Jan 24, 2026

I'll fix this tomorrow.

@arianvp arianvp force-pushed the fix-ekcert-checking branch from fa66b8e to ed13fbe Compare January 25, 2026 18:19
require (
github.com/google/go-attestation v0.4.4-0.20220404204839-8820d49b18d9
github.com/google/go-tpm-tools v0.3.8
github.com/google/go-attestation v0.6.1-0.20260123045045-5514d09200e7
Copy link
Author

Choose a reason for hiding this comment

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

This is the (unreleased) commit that implements exactly what we want. google/go-attestation@5514d09

Copy link
Collaborator

@kfox1111 kfox1111 left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks! :)

@kfox1111
Copy link
Collaborator

Some testing failures

@arianvp
Copy link
Author

arianvp commented Jan 27, 2026

I've been trying to get the test suite working locally but it seems incompatible with openssl 3 and my distro doesn't ship anything else...

@arianvp
Copy link
Author

arianvp commented Jan 27, 2026

Okay I'll try to get this fixed. :)

@arianvp
Copy link
Author

arianvp commented Jan 27, 2026

Ah seems CI is running in ubuntu 20.04 which is not even supported anymore. Seems there is quite a bit of work to clean this up. I doubt the test suite even passes on master on newer versions at the moment

@kfox1111
Copy link
Collaborator

quite possible. The tests were inherited from ages ago.

@arianvp arianvp force-pushed the fix-ekcert-checking branch from 3660a48 to bd61580 Compare January 27, 2026 22:01
@arianvp
Copy link
Author

arianvp commented Jan 27, 2026

I commited fixes and ran the tests locally on NixOS and they pass. I naively bumped the CI dockerfile to 24.04 in the hope that that fixes it in CI too

Copy link

@wagdav wagdav left a comment

Choose a reason for hiding this comment

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

I looked into the build errors and I left some suggestions to fix it. Hope it helps to get this merged!

We use (unreleased) new version of go-attestation 
which implements this for us. So we get it for free

Fix EKCert checking

We simply use the newly added functionality for this in go-attestation
@arianvp arianvp force-pushed the fix-ekcert-checking branch from bd61580 to a41192f Compare February 11, 2026 12:17
@arianvp
Copy link
Author

arianvp commented Feb 11, 2026

The whole ci seems to be a mess frankly. Giving another shot to clean it up with your suggestions but I have very little tolerance having to deal with docker so if it doesn't work I'll offer switching to Nix for CI or someone else will have to push this PR over the finish line. I'm fine with just pointing my stuff to this branch. The code is working fine for me and all the tests pass 🤷

@arianvp
Copy link
Author

arianvp commented Feb 11, 2026

I installed podman and podman-compose now but I just get permission denied errors etc. I give up. Someone else can try and fix this.

Though offer to dump all this docker-compose stuff for doing this with nix still stands :)

@wagdav
Copy link

wagdav commented Feb 11, 2026

@arianvp Using Docker Desktop on Mac, I could build the CI image with your latest changes. The GitHub Actions is now waiting for a maintainer's approval to run.

🤞 🍀

@kfox1111
Copy link
Collaborator

I was never involved in the tests, so I'm shooting a little blind there, but it looks ok to me.

Copy link
Collaborator

@kfox1111 kfox1111 left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks! :)

@kfox1111 kfox1111 merged commit 86db606 into spiffe:master Feb 11, 2026
1 check passed
@arianvp
Copy link
Author

arianvp commented Feb 11, 2026

Wooohooo!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

could not verify cert: x509: unhandled critical extension

4 participants