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

Implementation of the cryptokeys REST-API (PUT, POST, DELETE) #4106

Merged
merged 19 commits into from Sep 6, 2016

Conversation

Projects
None yet
7 participants
@benj-zen
Contributor

benj-zen commented Jul 5, 2016

We are a group of students at the Freie Universität Berlin. Our team consists of @zervnet, @MrM0nkey, @taudor and @benj-zen.

We implemented the Rest-Api methods for the cryptokeys module. While doing that we realized we need to fix #706, so we can return the id of the added key e.g. using POST. As proposed in #706 pdnsutil prints the added key-id to stdout.

As suggested in the discussion in the IRC channel, we only implemented the (return-id-)functionality for the major backend-modules (gmysql, gpgsql, gsqlite3, godbc, bind).

Documentation and tests are included.

@pieterlexis

This comment has been minimized.

Show comment
Hide comment
@pieterlexis

pieterlexis Jul 5, 2016

Member

small doc nit: code is shown in normal font:

img

Member

pieterlexis commented Jul 5, 2016

small doc nit: code is shown in normal font:

img

@benj-zen

This comment has been minimized.

Show comment
Hide comment
@benj-zen

benj-zen Jul 5, 2016

Contributor

@pieterlexis Should we remove the 'json' and instead format the actual json-object as code?

Contributor

benj-zen commented Jul 5, 2016

@pieterlexis Should we remove the 'json' and instead format the actual json-object as code?

Show outdated Hide outdated pdns/dnsseckeeper.hh
@pieterlexis

This comment has been minimized.

Show comment
Hide comment
@pieterlexis
Member

pieterlexis commented Jul 5, 2016

@benj-zen yes 👍

Show outdated Hide outdated pdns/pdnsutil.cc
@zeha

This comment has been minimized.

Show comment
Hide comment
@zeha

zeha Jul 5, 2016

Collaborator

Suggestion: use INSERT INTO ... RETURNING for DBs that support it, and use INSERT INTO; SELECT last_insert_id() for those that don't. (If submitting multiple statements works?)

Collaborator

zeha commented Jul 5, 2016

Suggestion: use INSERT INTO ... RETURNING for DBs that support it, and use INSERT INTO; SELECT last_insert_id() for those that don't. (If submitting multiple statements works?)

@@ -97,6 +97,7 @@ class gPgSQLFactory : public BackendFactory
declare(suffix,"delete-names-query","","delete from records where domain_id=$1 and name=$2");
declare(suffix,"add-domain-key-query","", "insert into cryptokeys (domain_id, flags, active, content) select id, $1, $2, $3 from domains where name=$4");
declare(suffix,"get-last-inserted-key-id-query","", "select lastval()");

This comment has been minimized.

@zeha

zeha Jul 5, 2016

Collaborator

this could take the name of the sequence, similar to oracle

@zeha

zeha Jul 5, 2016

Collaborator

this could take the name of the sequence, similar to oracle

This comment has been minimized.

@benj-zen

benj-zen Jul 7, 2016

Contributor

lastval() doesn't take any arguments. Do you mean replacing it with currval(sequence)?

@benj-zen

benj-zen Jul 7, 2016

Contributor

lastval() doesn't take any arguments. Do you mean replacing it with currval(sequence)?

Show outdated Hide outdated pdns/dbdnsseckeeper.cc
{ "keytype", keyType },
{ "flags", (uint16_t)value.first.d_flags },
{ "dnskey", value.first.getDNSKEY().getZoneRepresentation() }
{ "type", "Cryptokey" },

This comment has been minimized.

@zeha

zeha Jul 5, 2016

Collaborator

whitespace noise

@zeha

zeha Jul 5, 2016

Collaborator

whitespace noise

try {
dses.push_back(makeDSFromDNSKey(zonename, value.first.getDNSKEY(), keyid).getZoneRepresentation());
} catch (...) {}
try {

This comment has been minimized.

@zeha

zeha Jul 5, 2016

Collaborator

wrong indent?

@zeha

zeha Jul 5, 2016

Collaborator

wrong indent?

Show outdated Hide outdated pdns/ws-auth.cc
DNSSECKeeper dk(&B);
DomainInfo di;
if (!B.getDomainInfo(zonename, di))
throw HttpBadRequestException();

This comment has been minimized.

@zeha

zeha Jul 5, 2016

Collaborator

a lot of this code could be shared with GET/PUT/...

@zeha

zeha Jul 5, 2016

Collaborator

a lot of this code could be shared with GET/PUT/...

Show outdated Hide outdated pdns/ws-auth.cc
Show outdated Hide outdated pdns/ws-auth.cc
Show outdated Hide outdated pdns/ws-auth.cc
Show outdated Hide outdated pdns/ws-auth.cc
Show outdated Hide outdated pdns/ws-auth.cc
Show outdated Hide outdated pdns/ws-auth.cc
Show outdated Hide outdated pdns/ws-auth.cc
Show outdated Hide outdated pdns/ws-auth.cc
if (keyOrZone)
dpk.d_flags = 257;
else
dpk.d_flags = 256;

This comment has been minimized.

@zeha

zeha Jul 5, 2016

Collaborator

IIRC there's nicer code for this in current pdnsutil, could you check?

@zeha

zeha Jul 5, 2016

Collaborator

IIRC there's nicer code for this in current pdnsutil, could you check?

This comment has been minimized.

@benj-zen

benj-zen Jul 7, 2016

Contributor

Actually this is exactly how it's done in pdnsutil.

@benj-zen

benj-zen Jul 7, 2016

Contributor

Actually this is exactly how it's done in pdnsutil.

Show outdated Hide outdated pdns/ws-auth.cc
Show outdated Hide outdated pdns/ws-auth.cc
Show outdated Hide outdated pdns/ws-auth.cc
Show outdated Hide outdated pdns/ws-auth.cc
Show outdated Hide outdated pdns/ws-auth.cc
Show outdated Hide outdated pdns/ws-auth.cc
Show outdated Hide outdated pdns/ws-auth.cc
Show outdated Hide outdated pdns/ws-auth.cc
@zeha

This comment has been minimized.

Show comment
Hide comment
@zeha

zeha Sep 5, 2016

Collaborator

Code duplication + minor nits; I think the duplication should be fixed, but all of that could be done in a followup pr.

Collaborator

zeha commented Sep 5, 2016

Code duplication + minor nits; I think the duplication should be fixed, but all of that could be done in a followup pr.

@pieterlexis

This comment has been minimized.

Show comment
Hide comment
@pieterlexis

pieterlexis Sep 5, 2016

Member

As master is now 'unstable' for 4.1, can you guys rebase this PR so we can (finally 😄) merge it and create an issue ticket for the remaining few issues.

Member

pieterlexis commented Sep 5, 2016

As master is now 'unstable' for 4.1, can you guys rebase this PR so we can (finally 😄) merge it and create an issue ticket for the remaining few issues.

@benj-zen

This comment has been minimized.

Show comment
Hide comment
@benj-zen

benj-zen Sep 6, 2016

Contributor

Allright 👍 The rebasing is done.

Contributor

benj-zen commented Sep 6, 2016

Allright 👍 The rebasing is done.

@pieterlexis pieterlexis merged commit 6b85380 into PowerDNS:master Sep 6, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@ahupowerdns ahupowerdns modified the milestone: auth-4.1.0 Feb 23, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment