Skip to content

wfe: Separately handle badSignature at JWS parse time#8091

Merged
jprenken merged 5 commits into
letsencrypt:mainfrom
orangepizza:parsetimealgocheck
Apr 8, 2025
Merged

wfe: Separately handle badSignature at JWS parse time#8091
jprenken merged 5 commits into
letsencrypt:mainfrom
orangepizza:parsetimealgocheck

Conversation

@orangepizza

@orangepizza orangepizza commented Apr 1, 2025

Copy link
Copy Markdown
Contributor

solve #8088

RFC8555 6.2 requires badSignatureAlgorithm on unacceptable JWS signing algorithm, but current boulder return malform:failed to parse jws error instead

Its because this only checks about JWS protected header's signature algorithm, current checkAlgorithm is while too late to catch parse time error but not redundant, as it checks against a key and signed message

@orangepizza orangepizza requested a review from a team as a code owner April 1, 2025 05:09
@orangepizza orangepizza requested a review from aarongable April 1, 2025 05:09
@jsha jsha changed the title separately handle badsignalgo at JWS parse time separately handle badSignature at JWS parse time Apr 3, 2025
@beautifulentropy

beautifulentropy commented Apr 4, 2025

Copy link
Copy Markdown
Member

Thanks for taking the time to submit this PR. We're working on a PR to https://github.com/go-jose/go-jose that will make this detection considerably easier: go-jose/go-jose#181

@beautifulentropy

Copy link
Copy Markdown
Member

#8106 pulls in an exported error that you can unwrap in the WFE.

Comment thread wfe2/verify.go Outdated
@beautifulentropy beautifulentropy requested review from jprenken and jsha and removed request for aarongable April 8, 2025 21:50
@beautifulentropy

Copy link
Copy Markdown
Member

Thanks for the contribution, I'll have another team member get their eyes on this code soon.

Comment thread wfe2/verify.go Outdated
Co-authored-by: Samantha Frank <hello@entropy.cat>
@beautifulentropy beautifulentropy changed the title separately handle badSignature at JWS parse time wfe: Separately handle badSignature at JWS parse time Apr 8, 2025

@jsha jsha left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Sweet, thanks for spotting this and fixing it! 🎉

@jprenken jprenken merged commit 5cc8a77 into letsencrypt:main Apr 8, 2025
@orangepizza orangepizza deleted the parsetimealgocheck branch April 9, 2025 02:30
mcpherrinm pushed a commit to letsencrypt/pebble that referenced this pull request May 7, 2025
Pebble didn't have badSignatureAlgorithm error, while RFC8555 section
6.2 mandates it.

jws parse time check for bad algorithm is needed because same as
jose.parsesigned refuese to parse without algolist(which is good thing)
so error already thrown at that time and we need to catch this type of
error there.

see related boulder PR
letsencrypt/boulder#8091

---------

Co-authored-by: Aaron Gable <aaron@aarongable.com>
sheurich pushed a commit to sheurich/boulder that referenced this pull request Jun 13, 2025
solve letsencrypt#8088

RFC8555 6.2 requires badSignatureAlgorithm on unacceptable JWS signing
algorithm, but current boulder return malform:failed to parse jws error
instead

Its because this only checks about JWS protected header's signature
algorithm, current checkAlgorithm is while too late to catch parse time
error but not redundant, as it checks against a key and signed message

---------

Co-authored-by: Samantha Frank <hello@entropy.cat>
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.

4 participants