Opened 9 years ago

Closed 8 years ago

#1136 closed enhancement (fixed)

RR type implementation: SSHFP

Reported by: shane Owned by: muks
Priority: low Milestone: Sprint-20120320
Component: libdns++ Version:
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity: N/A
Sub-Project: DNS Feature Depending on Ticket:
Estimated Difficulty: 0.0 Add Hours to Ticket: 0
Total Hours: 0.88 Internal?: no

Description

Implement the SSHFP type, from RFC 4255.

See ticket #809 for more discussion.

Subtickets

Change History (12)

comment:1 Changed 9 years ago by shane

  • Milestone Next-Sprint-Proposed deleted

comment:2 Changed 8 years ago by muks

  • Owner changed from UnAssigned to muks
  • Status changed from new to assigned

Picking

comment:3 Changed 8 years ago by muks

  • Owner changed from muks to UnAssigned
  • Status changed from assigned to reviewing

Ready for review.

More wire tests can be added. But someone should review it now.

comment:4 follow-up: Changed 8 years ago by jinmei

Hmm, until now I've not realized this was not in the current sprint.

How did it end up getting picked up?

comment:5 Changed 8 years ago by muks

  • Milestone set to Sprint-20120320

Setting milestone to current sprint.

comment:6 in reply to: ↑ 4 Changed 8 years ago by shane

Replying to jinmei:

Hmm, until now I've not realized this was not in the current sprint.

How did it end up getting picked up?

Sorry, Mukund is coming up to speed and Jelte thought that a RR type implementation would be a nice introduction to libdns++, and I agreed. We should have added it to the sprint though!

comment:7 Changed 8 years ago by vorner

  • Owner changed from UnAssigned to vorner

comment:8 follow-up: Changed 8 years ago by vorner

  • Owner changed from vorner to muks

First, I think this should have a changelog.

And few notes:

  • The tests fail, with this error:
    [ RUN      ] Rdata_SSHFP_Test.toText
    rdata_sshfp_unittest.cc:69: Failure
    Value of: rdata_sshfp.toText()
      Actual: "2 1 123456789ABCDEF67890123456789ABCDEF67890"
    Expected: "2 1 123456789abcdef67890123456789abcdef67890"
    [  FAILED  ] Rdata_SSHFP_Test.toText (0 ms)
    
  • Could you provide a test where you test with more than one space to separate the items and/or different capitalization?
  • You have a generated file in git (src/lib/dns/tests/testdata/rdata_sshfp_fromWire.wire).
  • Maybe it could be convenient to have static functions for named constants for the alorithm and hash.
  • There's a file with copyright set to 2010.

comment:9 in reply to: ↑ 8 Changed 8 years ago by muks

Hi vorner

Thank you for the review. Comments are below:

Replying to vorner:

First, I think this should have a changelog.

Done :)

And few notes:

  • The tests fail, with this error:
    [ RUN      ] Rdata_SSHFP_Test.toText
    rdata_sshfp_unittest.cc:69: Failure
    Value of: rdata_sshfp.toText()
      Actual: "2 1 123456789ABCDEF67890123456789ABCDEF67890"
    Expected: "2 1 123456789abcdef67890123456789abcdef67890"
    [  FAILED  ] Rdata_SSHFP_Test.toText (0 ms)
    

Eek! Thought I had fixed that. The testcase was wrong.. I've added a case insensitive compare now.

  • Could you provide a test where you test with more than one space to separate the items and/or different capitalization?

Added more tests. :)

  • You have a generated file in git (src/lib/dns/tests/testdata/rdata_sshfp_fromWire.wire).

Actually, no.. it was a generated file, but in this case the .spec file for that particular testcase is not checked in. But the comment that it was generated stayed in the file. I've removed the comment now, and also removed the .wire extension to match other static wire data in that dir.

There are two other .spec files.. they need tests added. I've just kept them to test the wiredata generator script.

  • Maybe it could be convenient to have static functions for named constants for the alorithm and hash.

Other RR types don't do this either. Maybe we should have a new bug to check all of them.

  • There's a file with copyright set to 2010.

Changed the copyright notice in sshfp_44.h :)

comment:10 Changed 8 years ago by muks

  • Owner changed from muks to vorner

comment:11 Changed 8 years ago by vorner

  • Owner changed from vorner to muks
  • Total Hours changed from 0 to 0.88

OK, thanks. This seems good to merge. Now to make sure every ssh server I use has one! ;-).

comment:12 Changed 8 years ago by muks

  • Resolution set to fixed
  • Status changed from reviewing to closed

Pushed to master:

* 5cdde56 [1136] Add ChangeLog entry
* 5a045df [1136] Add rdata_sshfp_fromWire2 to EXTRA_DIST
* 3d8de45 [1136] Fix year in copyright notice
* ec745db [1136] Add more tests from review
* cebaf16 [1136] Remove message about the wire file being a generated one
* 9ef6f17 [1136] Rename non-generated wire data file
* 8cff3a6 [1136] Fix testcase to do case insensitive compare
* 8a0d8d0 [1136] Extra dist one more file
* abca9b6 Fix comment in MX testcase
* ea5ac57 [1136] Implement the SSHFP type, from RFC 4255

Resolving as fixed.

Note: See TracTickets for help on using tickets.