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

Extend certcheck to cope with Android attestation extension #865

Open
wants to merge 12 commits into
base: master
Choose a base branch
from

Conversation

daviddrysdale
Copy link
Contributor

@daviddrysdale daviddrysdale commented Nov 7, 2021

Android includes support for hardware-backed cryptographic keys. When asymmetric keys are generated in this system, the public key is exported in an X.509 certificate, which may be signed by hardware-protected device keys. The exported X.509 certificate may also include an Android-specific extension that attests to various features of the key and the device, described at https://developer.android.com/training/articles/security-key-attestation

This PR adds support for parsing this Android-specific extension, and along the way also improves support for dealing with certificates holding curve 25519 keys (Ed25519 or X25519).

@pav-kv
Copy link
Contributor

pav-kv commented Apr 29, 2022

@daviddrysdale Could you add some background/motivation for this PR in the description?

@pav-kv
Copy link
Contributor

pav-kv commented Apr 29, 2022

/gcbrun

@daviddrysdale
Copy link
Contributor Author

Updated the text above.

Copy link
Contributor

@pav-kv pav-kv left a comment

Choose a reason for hiding this comment

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

A couple of style nits.

x509util/files.go Show resolved Hide resolved
x509util/android.go Show resolved Hide resolved
@pav-kv
Copy link
Contributor

pav-kv commented Apr 29, 2022

@daviddrysdale Are these certs related to CT? Or is the idea of x509/x509util packages that they are general purpose, so any possible "official" extension can be added in it?

@daviddrysdale
Copy link
Contributor Author

Are these certs related to CT?

No, they're not in the Web PKI.

Or is the idea of x509/x509util packages that they are general purpose, so any possible "official" extension can be added in it?

The x509 package is definitely general, as it's a fork of the main Golang package. I tried to make x509util general too, although I imagine some CT-specific stuff might have crept in.

@pav-kv
Copy link
Contributor

pav-kv commented May 3, 2022

/gcbrun

Copy link
Contributor

@pav-kv pav-kv left a comment

Choose a reason for hiding this comment

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

There is some trouble with the CI build. Could you rebase to see if it helps to fix it?

Is this functional already in upstream x509? If not, any reason to have it in the fork first (except maybe convenience)? I'm happy to approve this PR, but I'm not sure who of this repo maintainers would follow further updates, particularly given they are defined in Andriod docs.

@AlCutter @getagit What is your view on this PR and the state of x509 fork? We previously discussed factoring it out to a separate repo, and/or finding an owner. I guess this discussion is relevant too.

x509util/android.go Outdated Show resolved Hide resolved
@daviddrysdale
Copy link
Contributor Author

Rebased on master

@pav-kv
Copy link
Contributor

pav-kv commented May 3, 2022

/gcbrun

@daviddrysdale
Copy link
Contributor Author

Is this functional already in upstream x509? If not, any reason to have it in the fork first (except maybe convenience)?

x509util/ is entirely local to this repo, it's only x509/ that's a fork – adding it here is entirely convenience because I'm used to using certcheck (as I wrote it).

@@ -0,0 +1,405 @@
// Copyright 2021 Google LLC. All Rights Reserved.
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a way to unit-test this file (and this PR in general)?

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 normally just manually run certcheck --verbose over some cert files…

@roger2hk
Copy link
Contributor

roger2hk commented Sep 2, 2022

/gcbrun

@daviddrysdale daviddrysdale force-pushed the android-ext branch 2 times, most recently from cfdee22 to f94dae6 Compare February 27, 2024 18:15
@daviddrysdale daviddrysdale force-pushed the android-ext branch 2 times, most recently from 71737a1 to f2fa8cb Compare March 26, 2024 15:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants