Opened 7 years ago

Closed 6 years ago

#2762 closed defect (fixed)

unable to add a TSIG key with algorithm HMAC-MD5

Reported by: cas Owned by: muks
Priority: medium Milestone: Sprint-20130917
Component: ~bind-ctl (obsolete) Version:
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity: Medium
Sub-Project: Core Feature Depending on Ticket:
Estimated Difficulty: 3 Add Hours to Ticket: 0
Total Hours: 0 Internal?: no

Description

shouldn't this work?

> config add tsig_keys/keys "test.key:EE1Pzk2V2xNi9N1Ms5h1JQ==:hmac-md5"
["login success "] login as cas
> config commit
Error: TSIG: TSIG key with unknown algorithm has non empty secret: test.key:EE1Pzk2V2xNi9N1Ms5h1JQ==:hmac-md5
Configuration not committed

the same with SHA1 works;

> config add tsig_keys/keys "test.key2:EE1Pzk2V2xNi9N1Ms5h1JQ==:hmac-sha1"
> config commit

Subtickets

Change History (15)

comment:1 follow-up: Changed 7 years ago by jinmei

unfortunately the key name has to be the counter-intuitive domain name
hmac-md5.sig-alg.reg.int. but we might be more lenient as there should be
no possibility of ambiguity.

comment:2 in reply to: ↑ 1 Changed 7 years ago by cas

Replying to jinmei:

unfortunately the key name has to be the counter-intuitive domain name
hmac-md5.sig-alg.reg.int. but we might be more lenient as there should be
no possibility of ambiguity.

while "hmac-md5.sig-alg.reg.int." is the official name, both ISC DHCP and BIND 9 (and other DNS products) just use "hmac-md5"

comment:3 Changed 7 years ago by shane

  • Defect Severity changed from N/A to Medium
  • Milestone changed from New Tasks to Next-Sprint-Proposed

I don't know that we even need to support the official name. I doubt there will be too many people demanding that we support the "hmac-md5.sig-alg.reg.int." label. :)

comment:4 Changed 7 years ago by shane

  • Milestone changed from Previous-Sprint-Proposed to Next-Sprint-Proposed

comment:5 Changed 7 years ago by jinmei

  • Milestone set to Next-Sprint-Proposed

comment:6 Changed 7 years ago by shane

  • Milestone set to Next-Sprint-Proposed

comment:7 Changed 7 years ago by shane

So it looks like the main change needs to happen in tsigkey.cc:

const
Name& TSIGKey::HMACMD5_NAME() {
    static Name alg_name("hmac-md5.sig-alg.reg.int");
    return (alg_name);
}

However, the hmac-md5.sig-alg.reg.int literally appears in many places throughout the code. :(

So, probably we also want to add a special case in the ConvertAlgorithmName() function:

    HashAlgorithm
    convertAlgorithmName(const isc::dns::Name& name) {
        if (name == TSIGKey::HMACMD5_NAME()) {
            return (isc::cryptolink::MD5);
        }
        // also get the official MD5 name
        if (name == "hmac-md5.sig-alg.reg.int") {
            return (isc::cryptolink::MD5);
        }

We can probably leave the rest of the code as it is, but will need to add a test case or two with the "hmac-md5" name.

comment:8 Changed 6 years ago by muks

  • Milestone changed from Next-Sprint-Proposed to Sprint-20130917
  • Owner set to UnAssigned
  • Status changed from new to reviewing

I had to read this up when trying to pick tickets for dclink, but I read through enough of the code to directly fix it.

Up for review.

comment:9 Changed 6 years ago by muks

  • Owner changed from UnAssigned to muks

Ugh this doesn't build with the new patch. :(

Checking why.

comment:10 Changed 6 years ago by muks

  • Owner changed from muks to UnAssigned

Back to review.

comment:11 Changed 6 years ago by vorner

  • Owner changed from UnAssigned to vorner

comment:12 Changed 6 years ago by vorner

  • Owner changed from vorner to muks

It seems OK to merge. But what about a changelog? It is visible change of interface and the ticket was reported by user, so I think it is needed.

comment:13 Changed 6 years ago by muks

  • Owner changed from muks to vorner

Hi Michal

Good catch. How does this look:

+XYZ.   [bug]           muks
+       We now also allow the short name ("hmac-md5")---along with the
+       long name ("hmac-md5.sig-alg.reg.int") that was allowed before for
+       HMAC-MD5---so that it is more conveninent to configure TSIG keys
+       using it.
+       (Trac #2762, git ...)

comment:14 Changed 6 years ago by vorner

  • Owner changed from vorner to muks

The text is OK, though I'm not sure about the use tripple dashes in our changelog.

comment:15 Changed 6 years ago by muks

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

Merged to master branch in commit c543008573eba65567e9c189824322954c6dd43b:

* b5fe9ef [2762] Update tests and implementation (see full log)
* 6a7aa9c [2762] Allow short names for HMAC-MD5

ChangeLog was also updated.

Resolving as fixed. Thank you for the reviews Michal.

Note: See TracTickets for help on using tickets.