feat(webauthncose): allow ber integers in ecdsa sigs#593
Merged
james-d-elliott merged 5 commits intomasterfrom Mar 1, 2026
Merged
feat(webauthncose): allow ber integers in ecdsa sigs#593james-d-elliott merged 5 commits intomasterfrom
james-d-elliott merged 5 commits intomasterfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds a package-scoped atomic flag and exported setter to toggle experimental allowance of non-canonical BER-encoded integers during ASN.1 unmarshaling of ECDSA signatures, updates signature verification to pass the corresponding asn1.Unmarshal option when enabled, and converts tests to assertion helpers while adding BER-focused test coverage. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #593 +/- ##
==========================================
+ Coverage 58.36% 58.44% +0.07%
==========================================
Files 42 43 +1
Lines 2613 2618 +5
==========================================
+ Hits 1525 1530 +5
Misses 864 864
Partials 224 224
🚀 New features to boost your workflow:
|
Contributor
There was a problem hiding this comment.
Pull request overview
Adds an opt-in mechanism to accept non-canonical ASN.1 BER INTEGER encodings in ECDSA (ES256) WebAuthn signatures to improve interoperability with non-compliant devices, by switching to an ASN.1 decoder that can be configured to permit BER integers.
Changes:
- Replace stdlib
encoding/asn1usage withgithub.com/go-webauthn/x/encoding/asn1and pass unmarshal options during ECDSA signature parsing. - Add a package-level experimental toggle (
SetExperimentalInsecureAllowBERIntegers) to allow BER-integer signatures. - Bump
github.com/go-webauthn/xdependency to v0.2.0.
Reviewed changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| protocol/webauthncose/webauthncose.go | Uses configurable ASN.1 unmarshal options during ECDSA signature parsing. |
| protocol/webauthncose/var.go | Introduces a global experimental toggle controlling BER-integer acceptance. |
| go.mod | Updates github.com/go-webauthn/x dependency version. |
| go.sum | Updates checksums for the bumped dependency set. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@protocol/webauthncose/var.go`:
- Around line 3-11: Replace the plain package-level bool allowBERIntegers with
an atomic.Bool and update the setter SetExperimentalInsecureAllowBERIntegers to
call Store(value) to avoid races; update readers (e.g. Verify in webauthncose.go
and any other places reading allowBERIntegers) to use Load() instead of direct
reads so all concurrent access is race-free and lock-free.
This allows implementers to permit the use of ASN.1 BER integers within signatures. Closes #408
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Signed-off-by: James Elliott <james-d-elliott@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Signed-off-by: James Elliott <james-d-elliott@users.noreply.github.com>
james-d-elliott
force-pushed
the
feat-asn1-ber
branch
from
4120d4a to
51f2d7a
Compare
james-d-elliott
changed the title
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@protocol/webauthncose/webauthncose_test.go`:
- Around line 51-75: The current TestOKPSignatureVerificationBER toggles
SetExperimentalInsecureAllowBERIntegers but exercises Ed25519
(OKPPublicKeyData), so it doesn't test BER-integer handling; add a new test (or
replace) that uses an EC2PublicKeyData with an ECDSA signature whose r and s use
non-minimal/BER-encoded integers, e.g., extra leading 0x00 bytes, then call
Verify(data, sig) with SetExperimentalInsecureAllowBERIntegers(false) and assert
Verify returns false (and no error if that’s the expected API), then enable
SetExperimentalInsecureAllowBERIntegers(true) and assert Verify returns true;
locate usage around TestOKPSignatureVerificationBER, reference EC2PublicKeyData,
SetExperimentalInsecureAllowBERIntegers, and the Verify method to implement the
signature construction and the two assertions.
- Around line 52-54: The test currently sets the global flag via
SetExperimentalInsecureAllowBERIntegers(true) and defers
SetExperimentalInsecureAllowBERIntegers(false), which can corrupt other tests;
capture the prior flag value into a local (e.g., prev :=
GetExperimentalInsecureAllowBERIntegers() or call
SetExperimentalInsecureAllowBERIntegers in a way that returns previous value)
before setting true, then defer restoring that saved value by calling
SetExperimentalInsecureAllowBERIntegers(prev) so the original state is
preserved; reference the SetExperimentalInsecureAllowBERIntegers function to
locate where to read/save and later restore the prior value.
ℹ️ Review info
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
protocol/attestation_tpm_test.goprotocol/client_test.goprotocol/webauthncose/webauthncose_test.go
james-d-elliott
force-pushed
the
feat-asn1-ber
branch
from
cf4c136 to
5a44260
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (2)
protocol/webauthncose/webauthncose_test.go (2)
431-437:⚠️ Potential issue | 🟠 MajorBER toggle path still isn’t meaningfully validated.
Line 431 enables BER integers, but this block re-verifies the same fixture signature and only checks success. That won’t catch regressions in BER-integer handling unless a non-minimal BER-encoded ECDSA signature is used with explicit off→fail and on→pass assertions.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@protocol/webauthncose/webauthncose_test.go` around lines 431 - 437, The test currently only enables BER support via SetExperimentalInsecureAllowBERIntegers(true) and re-runs VerifySignature(result, data, signature) which doesn’t validate BER handling; update the test to use a deliberately non-minimal BER-encoded ECDSA signature fixture (a signature variable different from the minimal one) and assert that VerifySignature(result, data, signature) fails when SetExperimentalInsecureAllowBERIntegers(false) (or not set) and succeeds when SetExperimentalInsecureAllowBERIntegers(true), keeping existing checks for err, ok, and result == tc.expected; locate the verification calls around VerifySignature and the toggle call to implement the off→fail and on→pass assertions.
431-439:⚠️ Potential issue | 🟡 MinorRestore previous BER flag state instead of hard-setting false.
Line 438 hard-resets global state. Preserve and restore the prior value so this test remains order-independent.
🔧 Suggested fix
- SetExperimentalInsecureAllowBERIntegers(true) + prevAllowBERIntegers := allowBERIntegers.Load() + SetExperimentalInsecureAllowBERIntegers(true) + defer SetExperimentalInsecureAllowBERIntegers(prevAllowBERIntegers) ok, err = VerifySignature(result, data, signature) assert.NoError(t, err) assert.True(t, ok) assert.Equal(t, tc.expected, result) - SetExperimentalInsecureAllowBERIntegers(false)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@protocol/webauthncose/webauthncose_test.go` around lines 431 - 439, The test currently calls SetExperimentalInsecureAllowBERIntegers(true) and then unconditionally SetExperimentalInsecureAllowBERIntegers(false), which can break test ordering; change the test to read and save the current flag value (via calling the getter or by storing the returned prior state if SetExperimentalInsecureAllowBERIntegers returns it), then set it to true, run VerifySignature(result, data, signature) and assertions, and finally restore the saved value by calling SetExperimentalInsecureAllowBERIntegers(savedValue) so the global BER flag state is preserved for other tests.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@protocol/webauthncose/webauthncose_test.go`:
- Around line 431-437: The test currently only enables BER support via
SetExperimentalInsecureAllowBERIntegers(true) and re-runs
VerifySignature(result, data, signature) which doesn’t validate BER handling;
update the test to use a deliberately non-minimal BER-encoded ECDSA signature
fixture (a signature variable different from the minimal one) and assert that
VerifySignature(result, data, signature) fails when
SetExperimentalInsecureAllowBERIntegers(false) (or not set) and succeeds when
SetExperimentalInsecureAllowBERIntegers(true), keeping existing checks for err,
ok, and result == tc.expected; locate the verification calls around
VerifySignature and the toggle call to implement the off→fail and on→pass
assertions.
- Around line 431-439: The test currently calls
SetExperimentalInsecureAllowBERIntegers(true) and then unconditionally
SetExperimentalInsecureAllowBERIntegers(false), which can break test ordering;
change the test to read and save the current flag value (via calling the getter
or by storing the returned prior state if
SetExperimentalInsecureAllowBERIntegers returns it), then set it to true, run
VerifySignature(result, data, signature) and assertions, and finally restore the
saved value by calling SetExperimentalInsecureAllowBERIntegers(savedValue) so
the global BER flag state is preserved for other tests.
ℹ️ Review info
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
protocol/attestation_tpm_test.goprotocol/client_test.goprotocol/webauthncose/webauthncose_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- protocol/attestation_tpm_test.go
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This allows implementers to permit the use of ASN.1 BER integers within ECDSA signatures. As noted by the function to enable this feature, this is experimental and considered insecure. The ambiguous nature of ASN.1 BER can lead to security issues, and as such this option is not recommended. It's purpose is to support non-conformant authenticators / credentials in the wild, though it's not necessary to support these per the spec. Use at your own risk.
Closes #408