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

[Enhancement Request] Is it possible to have a way to disable sha256-simd via an env var or some other method? #1697

Closed
tesshuflower opened this issue Sep 21, 2022 · 10 comments · Fixed by #1700
Assignees

Comments

@tesshuflower
Copy link

We are building restic (https://github.com/restic/restic) which uses minio-go - we're building to be FIPS-compatible, so that means it's using a version of goboringcrypto.

What we find is when running in FIPS enabled environments, when the CPU supports it, minio-go is using sha256-simd as a replacement for crypto/sha256. This ends up creating a hash with sha256.New() which goboringcrypto does not recognize.

        "panic: goboringcrypto: hmac hash not recognized",
        "",
        "goroutine 21 [running]:",
        "crypto/hmac.New(0x8?, {0xc0001f0a80?, 0x0?, 0x8?})",
        "    crypto/hmac/hmac.go:136 +0x229",
        "github.com/minio/minio-go/v7/pkg/signer.sumHMAC({0xc0001f0a80?, 0xc0001f0a78?, 0x8?}, {0xc0001f0a90, 0x8, 0x8})",
        "    github.com/minio/minio-go/v7@v7.0.14/pkg/signer/utils.go:40 +0x4a",
        "github.com/minio/minio-go/v7/pkg/signer.getSigningKey({0xc00003a016?, 0x3?}, {0xe9ec5b, 0x9}, {0x9?, 0xc00019a480?, 0x0?}, {0xe98422, 0x2})",
        "    github.com/minio/minio-go/v7@v7.0.14/pkg/signer/request-signature-v4.go:68 +0xfc",
        "github.com/minio/minio-go/v7/pkg/signer.signV4({{0xe986bb, 0x3}, 0xc000284480, {0xe9d11d, 0x8}, 0x1, 0x1, 0xc0001f9e00, {0x0, 0x0}, ...}, ...)",
        "    github.com/minio/minio-go/v7@v7.0.14/pkg/signer/request-signature-v4.go:289 +0x55d",
        "github.com/minio/minio-go/v7/pkg/signer.SignV4(...)",
        "    github.com/minio/minio-go/v7@v7.0.14/pkg/signer/request-signature-v4.go:317",
        "github.com/minio/minio-go/v7.Client.getBucketLocationRequest({0xc000284240, 0xc000122cc0, 0x0, {{0x0, 0x0}, {0x0, 0x0}}, 0x0, 0xc0001f9c80, 0xc000124be0, ...}, ...)",
        "    github.com/minio/minio-go/v7@v7.0.14/bucket-cache.go:251 +0x770",
        "github.com/minio/minio-go/v7.Client.getBucketLocation({0xc000284240, 0xc000122cc0, 0x0, {{0x0, 0x0}, {0x0, 0x0}}, 0x0, 0xc0001f9c80, 0xc000124be0, ...}, ...)",
        "    github.com/minio/minio-go/v7@v7.0.14/bucket-cache.go:100 +0x118",
        "github.com/minio/minio-go/v7.Client.newRequest({0xc000284240, 0xc000122cc0, 0x0, {{0x0, 0x0}, {0x0, 0x0}}, 0x0, 0xc0001f9c80, 0xc000124be0, ...}, ...)",
        "    github.com/minio/minio-go/v7@v7.0.14/api.go:766 +0xde",
        "github.com/minio/minio-go/v7.Client.executeMethod({0xc000284240, 0xc000122cc0, 0x0, {{0x0, 0x0}, {0x0, 0x0}}, 0x0, 0xc0001f9c80, 0xc000124be0, ...}, ...)",
        "    github.com/minio/minio-go/v7@v7.0.14/api.go:637 +0x458",
        "github.com/minio/minio-go/v7.Client.listObjectsV2Query({0xc000284240, 0xc000122cc0, 0x0, {{0x0, 0x0}, {0x0, 0x0}}, 0x0, 0xc0001f9c80, 0xc000124be0, ...}, ...)",
        "    github.com/minio/minio-go/v7@v7.0.14/api-list.go:206 +0x7b4",
        "github.com/minio/minio-go/v7.Client.listObjectsV2.func1(0xc000122d20)",
        "    github.com/minio/minio-go/v7@v7.0.14/api-list.go:98 +0x24e",
        "created by github.com/minio/minio-go/v7.Client.listObjectsV2",
        "    github.com/minio/minio-go/v7@v7.0.14/api-list.go:92 +0x2ba",
        "ERROR: failure checking existence of repository"

Is there a way to allow an env var or something equivalent to allow us to disable the usage of the sha256-simd library (i.e. fallback to crypto/sha256 for certain envs?

For reference, we've seen that syncthing has implemented a workaround to disable the library via an env var:
syncthing/syncthing#3617

@harshavardhana
Copy link
Member

We should not fail in this scenario, we should check why boring crypto is resulting in unexpected output.

@tesshuflower
Copy link
Author

I think this is the place:

goboring tries to recognize the specific HMAC that is being used:
https://github.com/golang/go/blob/dev.boringcrypto.go1.18/src/crypto/internal/boring/hmac.go (hashToMD)

We're seeing that hashToMD is returning nil.

@harshavardhana
Copy link
Member

Okay that is quite terrible :-)

@harshavardhana
Copy link
Member

goboring tries to recognize the specific HMAC that is being used: https://github.com/golang/go/blob/dev.boringcrypto.go1.18/src/crypto/internal/boring/hmac.go (hashToMD)

@aead @klauspost ^^ FYI when FIPS is compiled in.

@klauspost
Copy link
Contributor

klauspost commented Sep 26, 2022

@tesshuflower The CustomSHA256 in Client options allows you to specify which sha256 hash to use.

The interface is easy to satisfy:

type Hasher interface {
	hash.Hash
	Close()
}

@tesshuflower
Copy link
Author

@klauspost I think even if I try to set CustomSHA256 in the the client options, it still ends up calling utilities in pkg/signer that use the sha256-simd?

https://github.com/minio/minio-go/blob/master/pkg/signer/utils.go#L40

This seems to be called from bucket_cache:
https://github.com/minio/minio-go/blob/master/bucket-cache.go#L257

and I don't see where it will use the customsha256 from options in this case - Possibly I'm missing something here though.

@klauspost
Copy link
Contributor

klauspost commented Sep 26, 2022

I see. That is a mistake.

I may add add fips build tag as well.

@aead
Copy link
Member

aead commented Sep 26, 2022

Yes, a fips build tag is needed. FIPS / boringcrypto requires that a specific implementation of SHA-256 is used. A "custom" SHA-256 implementation (like SHA256-simd) will not work.

@klauspost
Copy link
Contributor

With #1700 You can

  1. Use build tag fips

or

  1. Replace CustomSHA256. The custom sha256 will not be used for signatures.

@tesshuflower
Copy link
Author

Wow this is great! Thanks very much for the quick turnaround!

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 a pull request may close this issue.

4 participants