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

RRSIG parsing does not accept algorithm code in mnemonic form #1447

Closed
ghost opened this issue Apr 3, 2023 · 7 comments · Fixed by #1456
Closed

RRSIG parsing does not accept algorithm code in mnemonic form #1447

ghost opened this issue Apr 3, 2023 · 7 comments · Fixed by #1456

Comments

@ghost
Copy link

ghost commented Apr 3, 2023

https://www.rfc-editor.org/rfc/rfc4034.html#section-3.2

3.2.  The RRSIG RR Presentation Format

   The presentation format of the RDATA portion is as follows:

   The Type Covered field is represented as an RR type mnemonic.  When
   the mnemonic is not known, the TYPE representation as described in
   [RFC3597], Section 5, MUST be used.

   The Algorithm field value MUST be represented either as an unsigned
   decimal integer or as an algorithm mnemonic, as specified in Appendix
   A.1.

ZoneParser does not accept the mnemonic form of the "Algorithm field" for RRSIG records.

e.g. example.com. 3600 IN RRSIG A RSASHA256 ... should be accepted.

It looks like func (rr *RRSIG) parse(c *zlexer, o string) *ParseError {...} in scan_rr.go needs to be updated to parse algorithm mnemonics the same as exists for CERT, DS, and TA RR types.

@vdukhovni
Copy link

Yes, parsing known mnemonics avoids some interoperability issues with systems that output mnemonics in the presentation form. Sadly, that also means ongoing maintenance of the known mnemonics list.

Superficially, this is not too different from similar ongoing bitrot as new RRtypes appear, however new RRtypes necessarily imply new code to support non-opaque parsing of the RDATA, while RRSIG algorithms don't affect the structure of the RRSIG RDATA. So it would have been better if the RFC text did NOT endorse mnemonics here, they needlessly complicate parsing. Sadly, my time machine is undergoing maintenance in the far future...

@miekg
Copy link
Owner

miekg commented Apr 18, 2023

Thanks @vdukhovni. I can't remember the exact reason for not implementing this (may I didn't read the relevant paragraph in the rfc at the time), but your comment makes it clear why this shouldn't be implemented.

I may actually put this as a comment somewhere for documentation purposes.

@miekg miekg added the wont-fix label Apr 27, 2023
@miekg miekg closed this as completed Apr 27, 2023
@vdukhovni
Copy link

FWIW, this means that Java's default output form of RRSIGs can't be parsed by Go. :-(

@miekg
Copy link
Owner

miekg commented Apr 27, 2023 via email

miekg added a commit that referenced this issue Apr 27, 2023
Java outputs these *and* the RFC says we should parse them, so parse
them. We'll never output them though. Throwback to the "be lenient to
what you accept, but strict with what you output". Anyhow the diff is
tiny and it helps interop.

Fixes: #1447

Signed-off-by: Miek Gieben <miek@miek.nl>
@vdukhovni
Copy link

Ugh.....

On Thu, 27 Apr 2023, 17:02 Viktor Dukhovni, @.> wrote: FWIW, this means that Java's default output form of RRSIGs can't be parsed by Go. :-( — Reply to this email directly, view it on GitHub <#1447 (comment)>, or unsubscribe https://github.com/notifications/unsubscribe-auth/AACWIWZLW3N4UY3JI6Z62Y3XDKDA5ANCNFSM6AAAAAAWRTVMI4 . You are receiving this because you modified the open/close state.Message ID: @.>

@evansrg Suggested out of band that perhaps my claim was hasty. Perhaps the observed presentation form was not from the core Java RRSIG class, but from a particular application library. So perhaps closed will do for now.

@miekg miekg removed the wont-fix label Apr 28, 2023
@miekg
Copy link
Owner

miekg commented Apr 28, 2023

#1456 is tiny enough, so we should prolly just merge that

@miekg miekg reopened this Apr 28, 2023
@ghost
Copy link
Author

ghost commented Apr 28, 2023

+1 to merging #1456. At least for our use case this would be helpful for interoperability.

miekg added a commit that referenced this issue Apr 28, 2023
* Allow RRSIG algorithm mnemonics

Java outputs these *and* the RFC says we should parse them, so parse
them. We'll never output them though. Throwback to the "be lenient to
what you accept, but strict with what you output". Anyhow the diff is
tiny and it helps interop.

Fixes: #1447

Signed-off-by: Miek Gieben <miek@miek.nl>

* Check parsed algorithm

Signed-off-by: Miek Gieben <miek@miek.nl>

---------

Signed-off-by: Miek Gieben <miek@miek.nl>
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.

2 participants