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

Device attestation #977

Merged
merged 40 commits into from Sep 8, 2022
Merged

Device attestation #977

merged 40 commits into from Sep 8, 2022

Conversation

maraino
Copy link
Contributor

@maraino maraino commented Jul 15, 2022

Description

Support for ACME device-attest-01 challenge. See:

We need to add SAN support for permanent identifiers in go.step.sm/crypto. There's some work by @brandonweeks here. We can get rid of github.com/google/go-attestation dependency by encoding the SANs manually, extending the new SANs of smallstep/crypto#27

@CLAassistant
Copy link

CLAassistant commented Jul 15, 2022

CLA assistant check
All committers have signed the CLA.

@github-actions github-actions bot added the needs triage Waiting for discussion / prioritization by team label Jul 15, 2022
@maraino
Copy link
Contributor Author

maraino commented Jul 15, 2022

It's possible to test this by installing a profile with the root certificate, it can be done by visiting the roots.pem endpoint (https://ca.local/roots.pem) and then installing the ACMECertificate profile (acme.mobileconfig) using the Apple Configurator app:

<?xml version=”1.0” encoding=”UTF-8”?>
<!DOCTYPE plist PUBLIC “-//Apple//DTD PLIST 1.0//EN” “http://www.apple.com/DTDs/PropertyList-1.0.dtd”>
<plist version=”1.0”>
    <dict>
        <key>PayloadVersion</key>
        <integer>1</integer>
        <key>PayloadUUID</key>
        <string>Ignored</string>
        <key>PayloadType</key>
        <string>Configuration</string>
        <key>PayloadDisplayName</key>
        <string>ACME</string>
        <key>PayloadIdentifier</key>
        <string>com.example.myprofile</string>
        <key>PayloadContent</key>
        <array>
            <dict>
                <key>ClientIdentifier</key>
                <string>YOUR DEVICE UDID OR SERIAL NUMBER</string>
                <key>ExtendedKeyUsage</key>
                <array>
                    <string>1.3.6.1.5.5.7.3.2</string>
                </array>
                <key>HardwareBound</key>
                <true/>
                <key>Attest</key>
                <true/>
                <key>KeySize</key>
                <integer>384</integer>
                <key>KeyType</key>
                <string>ECSECPrimeRandom</string>
                <key>KeyUsage</key>
                <integer>5</integer>
                <key>PayloadIdentifier</key>
                <string>com.example.myacmepayload</string>
                <key>PayloadType</key>
                <string>com.apple.security.acme</string>
                <key>PayloadUUID</key>
                <string>cbdc6238-feec-4171-878d-34e576bbb813</string>
                <key>PayloadVersion</key>
                <integer>1</integer>
                <key>Subject</key>
                <array>
                    <array>
                        <array>
                            <string>C</string>
                            <string>US</string>
                        </array>
                    </array>
                    <array>
                        <array>
                            <string>O</string>
                            <string>Example Inc.</string>
                        </array>
                    </array>
                    <array>
                        <array>
                            <string>1.2.840.113635.100.6.99999.99999</string>
                            <string>test custom OID value</string>
                        </array>
                    </array>
                </array>
                <key>SubjectAltName</key>
                <dict>
                    <key>dNSName</key>
                    <string>site.example.com</string>
                    <key>ntPrincipalName</key>
                    <string>site.example.com</string>
                </dict>
                <key>DirectoryURL</key>
                <string>https://ca.local/acme/acme/directory</string>
            </dict>
        </array>
    </dict>
</plist>

The ClientIdentifier will become the identifier value; the current implementation expects this to match the serial number of the UDID in the attestation certificate. But we might want to use an external service to check this and get the list of SANs to set. Currently, a PermanentIdentifier SAN is added with that value.

@maraino maraino marked this pull request as draft Jul 15, 2022
@maraino maraino marked this pull request as ready for review Sep 6, 2022
@maraino maraino requested a review from hslatman Sep 6, 2022
@maraino
Copy link
Contributor Author

maraino commented Sep 6, 2022

@hslatman I think it is a good idea to start merging this; it will make it easy to improve on it. For example, to add tests on the new methods it will be nice to merge the attestation roots part too and use minica to create fake attestation certificates.

acme/api/handler.go Outdated Show resolved Hide resolved
acme/api/order_test.go Outdated Show resolved Hide resolved
acme/challenge.go Outdated Show resolved Hide resolved
acme/challenge.go Outdated Show resolved Hide resolved
acme/errors.go Outdated Show resolved Hide resolved
acme/order.go Outdated Show resolved Hide resolved
authority/provisioner/acme.go Outdated Show resolved Hide resolved
// AuthorizeChallenge checks if the given challenge is enabled. By default
// http-01, dns-01 and tls-alpn-01 are enabled, to disable any of them the
// Challenge provisioner property should have at least one element.
func (p *ACME) AuthorizeChallenge(ctx context.Context, challenge string) error {
Copy link
Member

@hslatman hslatman Sep 8, 2022

Choose a reason for hiding this comment

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

I think that for this specific function we don't necessarily need to be consistent with function names or arguments that exist in other provisioners. The ACME one is already somewhat different than the others, anyway. IsChallengeEnabled is a more logical name for this function, imo. I think the challenge can also be more strongly typed instead of using the string.

Copy link
Contributor Author

@maraino maraino Sep 8, 2022

Choose a reason for hiding this comment

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

The strongly typed thing is 59c5219.

I noticed custom methods in acme are using Authorize* form (AuthorizeOrderIdentifier).

Copy link
Contributor Author

@maraino maraino Sep 8, 2022

Choose a reason for hiding this comment

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

Renamed with fd4e96d

authority/provisioner/acme.go Outdated Show resolved Hide resolved
authority/provisioner/acme.go Outdated Show resolved Hide resolved
@maraino maraino requested a review from hslatman Sep 8, 2022
@maraino maraino merged commit b2119e9 into master Sep 8, 2022
6 checks passed
@maraino maraino deleted the device-attestation branch Sep 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs triage Waiting for discussion / prioritization by team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants