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

Add arbitrary tag capability to protobuf output in dnsdist #5396

Merged
merged 11 commits into from Aug 3, 2017

Conversation

Projects
None yet
6 participants
@sethomisc
Contributor

sethomisc commented Jun 13, 2017

Short description

New Lua commands for storing text table information in DNSQuestion and reading the table in DNSResponse. Additionally storing table in DNSProtoBufMessage as tags along with a function to alter the protobuf message from a 'query' to that of a 'response'. These added functions are to allow reporting of Lua based RPZ detection and reporting. Notes, example scripts and config file are located in the directory pdns/zzz-gca-example Tests will be coming soon.

Checklist

I have:

  • [ X] read the CONTRIBUTING.md document
  • [ X] compiled and tested this code
  • [ X] included documentation (including possible behaviour changes)
  • [ X] documented the code
  • added or modified regression test(s)
  • added or modified unit test(s)
  • [X ]
    dnsdist-Lua additions-June_7_2017.pdf
@johnhtodd

This comment has been minimized.

johnhtodd commented Jun 20, 2017

A better title for this PR would be "Add arbitrary tag capability to protobuf output in dnsdist"

@Habbie Habbie changed the title from Dnsdist mod2 to Add arbitrary tag capability to protobuf output in dnsdist Jun 20, 2017

@Habbie

This comment has been minimized.

Member

Habbie commented Jun 20, 2017

A better title for this PR would be "Add arbitrary tag capability to protobuf output in dnsdist"

Title updated!

@johnhtodd johnhtodd referenced this pull request Jun 20, 2017

Closed

WIP: Add dnstap-compatible protobuf support to dnsdist. #5201

1 of 6 tasks complete
@rgacogne

Hello!

Thank you for this pull request, I have written a few comments below. Other than that this pull request lacks documentation for the new methods in pdns/dnsdistdist/README.md and the whole zzz-gca-example directory should probably go (please don't commit .pdf or .abw files).

// --------------------------------------------------------------------------
// GCA - Seth Ornstein added lua callable functions - 6/2/2017
// DNSQuestion - setTag, setTagArray, getTagMatch, getTagArray

This comment has been minimized.

@rgacogne

rgacogne Jun 21, 2017

Member

Please remove this kind of comments, it doesn't really help and even make the code less readable.

This comment has been minimized.

@sethomisc

sethomisc Jun 30, 2017

Contributor

Comments removed.

This comment has been minimized.

@sethomisc

sethomisc Jun 30, 2017

Contributor

Comments removed.

g_lua.registerFunction<void(DNSQuestion::*)(vector<pair<string, string>>)>("setTagArray", [](DNSQuestion& dq, const vector<pair<string, string>>&tags) {
setLuaSideEffect();

This comment has been minimized.

@rgacogne

rgacogne Jun 21, 2017

Member

setLuaSideEffect() and setLuaNoSideEffect() only make sense for functions called from the command line so they show up in delta().

This comment has been minimized.

@sethomisc

sethomisc Jun 30, 2017

Contributor

calls to setLuaSideEffect() and setLuaNoSideEffect() removed.

message.setQueryTime(0, 0); // seconds and microseconds
message.addRRs(strQueryName); // add a RR to the response

This comment has been minimized.

@rgacogne

rgacogne Jun 21, 2017

Member

A method called setProtobufResponseType() should probably not add a RR, please move that to a separate one.

This comment has been minimized.

@sethomisc

sethomisc Jun 30, 2017

Contributor

New function setProtobufResponseRR() added. setProtobufResponseType() does not add a RR.

message.setType(DNSProtoBufMessage::Response); // set protobuf type to be response - not query
#ifdef TRASH

This comment has been minimized.

@rgacogne

rgacogne Jun 21, 2017

Member

I have the feeling this should have been removed before opening this PR :-)

This comment has been minimized.

@sethomisc

sethomisc Jun 30, 2017

Contributor

Removed #ifdef TRASH lines.

});
// setProtobufResponseTypeQT - with query time as function parameter
g_lua.registerFunction<void(DNSDistProtoBufMessage::*)(const std::string&, time_t sec, uint uSec)>("setProtobufResponseTypeTS", [](DNSDistProtoBufMessage& message, const std::string& strQueryName, time_t sec, uint uSec) {

This comment has been minimized.

@rgacogne

rgacogne Jun 21, 2017

Member

This method could be merge with the previous one using optional time parameters, same remark about adding a RR.

This comment has been minimized.

@sethomisc

sethomisc Jun 30, 2017

Contributor

setProtobufResponseType() now can take optional parameters to set time.

// ----------------------------------------------------------------------------
// count() - return number of tag entries
// ----------------------------------------------------------------------------
int count() const

This comment has been minimized.

@rgacogne

rgacogne Jun 21, 2017

Member

size_t or unsigned int

This comment has been minimized.

@sethomisc

sethomisc Jun 30, 2017

Contributor

Replaced with size_t

@@ -71,6 +196,7 @@ struct DNSQuestion
bool skipCache{false};
bool ecsOverride;
bool useECS{true};
QTag qTag; // GCA - Seth Ornstein - extra class for tags 5/30/2017

This comment has been minimized.

@rgacogne

rgacogne Jun 21, 2017

Member

Since we are trying to keep the DNSQuestion object as light as possible for performance reason, would you be willing to consider making that an std::unique_ptr<QTag> so that we don't pay the cost of constructing the object until we really use it? Please also move it up in the DNSQuestion object with the other pointers to make padding more optimal.
By the way, if I'm not mistaken, the QTag object is basically a std::unordered_map<std::string,std::string>, perhaps we could avoid defining a new object and just handle the needful in the Lua bindings?

// ----------------------------------------------------------------------------
void DNSProtoBufMessage::addTags(const std::string& strLabel, const std::string& strValue)
{

This comment has been minimized.

@rgacogne

rgacogne Jun 21, 2017

Member

I think we should move RecProtoBufMessage::setPolicyTags() and RecProtoBufMessage::getPolicyTags() here instead.

cTemp[2] = 0;
cTemp[3] = 1;
rr->set_rdata(cTemp, 4);
}

This comment has been minimized.

@rgacogne

rgacogne Jun 21, 2017

Member

This method only adds one RR, it should probably called addRR() instead. should Please don't hardcode the type, class, TTL and content here, those should be parameters passed to this method.

g_lua.registerFunction<string(DNSQuestion::*)(std::string)>("getTagMatch", [](const DNSQuestion& dq, const std::string& strLabel) {
std::string strValue = dq.qTag.getMatch(strLabel);
return(strValue);

This comment has been minimized.

@rgacogne

rgacogne Jun 21, 2017

Member

The parentheses are not needed for this kind of return statement.

This comment has been minimized.

@sethomisc

sethomisc Jun 30, 2017

Contributor

Parentheses removed for the return.

@sethomisc

This comment has been minimized.

Contributor

sethomisc commented Jun 30, 2017

Bert and Remi,
Thank you for your constructive comments. I have attempted to do all of them. In the structure DNSQuestion in dnsdist.hh I changed QTag to be a shared_ptr as I had a lot of difficulty in making it a unique_ptr. Please let me know if any of my changes are unclear or need further work.

Thanks,

Seth

@rgacogne

Thank you for making these changes! In addition to my individual comments on the code, it might make more sense to merge test_ProtobufTag.py with the existing test_Protobuf.py to avoid code duplication.

Documentation is also lacking for the new methods:

  • setTag(), setTagArray(), getTagMatch() (getTag()?), getTagArray() for the DNSQuestion object
  • setTag(), setTagArray(), setProtobufResponseType(), setProtobufResponseRR() (addResponseRR()) for the DNSDistProtoBufMessage one
@@ -1572,6 +1572,57 @@ vector<std::function<void(void)>> setupLua(bool client, const std::string& confi
}
});
g_lua.registerFunction<void(DNSQuestion::*)(std::string, std::string)>("setTag", [](DNSQuestion& dq, const std::string& strLabel, const std::string& strValue) {
if(dq.qTag == NULL)

This comment has been minimized.

@rgacogne

rgacogne Jul 3, 2017

Member

It would be a bit nicer to use nullptr instead of NULL, as the former can't be confused with an integer.

g_lua.registerFunction<void(DNSQuestion::*)(std::string, std::string)>("setTag", [](DNSQuestion& dq, const std::string& strLabel, const std::string& strValue) {
if(dq.qTag == NULL)
{

This comment has been minimized.

@rgacogne

rgacogne Jul 3, 2017

Member

We usually place the { following an if or else on the same line.

if(dq.qTag == NULL)
{
dq.qTag = std::shared_ptr<QTag>(new QTag);

This comment has been minimized.

@rgacogne

rgacogne Jul 3, 2017

Member

Replacing std::shared_ptr<QTag>(new QTag) by std::make_shared<QTag>() would save an allocation and be a bit more easy to read.

if(dq.qTag == NULL)
{
dq.qTag = std::shared_ptr<QTag>(new QTag);
}

This comment has been minimized.

@rgacogne

rgacogne Jul 3, 2017

Member

We usually don't indent the closing }

});
g_lua.registerFunction<void(DNSDistProtoBufMessage::*)(const std::string&, uint uType, uint uClass, uint uTTL, vector<pair<int, int>> )>("setProtobufResponseRR", [](DNSDistProtoBufMessage& message,

This comment has been minimized.

@rgacogne

rgacogne Jul 3, 2017

Member

Please use uint16_t for type and class, uint32_t for ttl. A simple std:string might be more suited for blobData, it would prevent to have to allocate an array and copy the content to it?

bool bStatus = true;
tagData.insert( {strLabel, strValue});
return(bStatus);

This comment has been minimized.

@rgacogne

rgacogne Jul 3, 2017

Member

It looks like it always return true, so perhaps the return value might be omitted?

std::unordered_map<std::string, std::string>::const_iterator got =tagData.find (strLabel);
if(got == tagData.end())
{
return("");

This comment has been minimized.

@rgacogne

rgacogne Jul 3, 2017

Member

We usually don't use parenthesis for return statements.

return;
PBDNSMessage_DNSResponse_DNSRR* rr = response->add_rrs();
if (rr) {

This comment has been minimized.

@rgacogne

rgacogne Jul 3, 2017

Member

Indentation looks off.

This comment has been minimized.

@rgacogne

rgacogne Jul 7, 2017

Member

still true

#endif /* HAVE_PROTOBUF */
}
void DNSProtoBufMessage::addRR(const std::string& strName, uint32_t uType, uint32_t uClass, uint32_t uTTL, const uint8_t *ptrBlob, size_t uBlobLen)

This comment has been minimized.

@rgacogne

rgacogne Jul 3, 2017

Member

uType and uClass should be uint16_t, ptrBlob and uBlobLen can be merged into a std::string.

});
g_lua.registerFunction<string(DNSQuestion::*)(std::string)>("getTagMatch", [](const DNSQuestion& dq, const std::string& strLabel) {

This comment has been minimized.

@rgacogne

rgacogne Jul 3, 2017

Member

Perhaps getTag() would be more logical, to match setTag()?

@sethomisc

This comment has been minimized.

Contributor

sethomisc commented Jul 6, 2017

Remi,
Thanks for your very constructive comments, I've gone through the list and corrected them.
I'm not clear on how I should document my changes, any direction on that would also be appreciated.
I've attached a very brief description of the new lua commands with examples below.
Seth

ReadMe-7-5-2017.txt

@pieterlexis

This comment has been minimized.

Member

pieterlexis commented Jul 7, 2017

@sethomisc I have created new documentation for the upcoming 4.1 release (#5481), you can PR documentation there. Or if you want, I can add your readme contents to them.

});
g_lua.registerFunction<void(DNSDistProtoBufMessage::*)(boost::optional <time_t> sec, boost::optional <uint> uSec)>("setProtobufResponseType",
[](DNSDistProtoBufMessage& message, boost::optional <time_t> sec, boost::optional <uint> uSec) {

This comment has been minimized.

@rgacogne

rgacogne Jul 7, 2017

Member

uSec should be a uint32_t here to be consistent with DNSProtoBufMessage::setQueryTime(). Be careful that you need to change it twice due to the LuaWrapper syntax.

@rgacogne

I realize I haven't said it before but I really like the features that this pull request implements, so thank you again. It's merge-able as it is, but if you don't mind fixing the few remaining nits it would be almost perfect. Regarding the documentation, you could just drop a few lines in pdns/README-dnsdist.md to document the member functions that you are adding. Look for DNSDistProtoBufMessage related and DNSQuestion related.
If you can't figure it out just let me know, I can fix the PR myself, or even merge it and fix the remaining issues afterward, but I think it would be more rewarding to you if you could do it yourself :-)

});
g_lua.registerFunction<void(DNSDistProtoBufMessage::*)(const std::string& strQueryName, uint uType, uint uClass, uint uTTL, const std::string& strBlob)>("addResponseRR", [](DNSDistProtoBufMessage& message,
const std::string& strQueryName, uint16_t uType, uint uClass, uint32_t uTTL, const std::string& strBlob) {

This comment has been minimized.

@rgacogne

rgacogne Jul 7, 2017

Member

uType should be uint16_t on the first line too. uClass should be uint16_t on both lines

std::string getEntry(size_t iEntry) const
{
std::string strEntry;
size_t iCounter = 0;

This comment has been minimized.

@rgacogne

rgacogne Jul 7, 2017

Member

small indentation issue

return;
PBDNSMessage_DNSResponse_DNSRR* rr = response->add_rrs();
if (rr) {

This comment has been minimized.

@rgacogne

rgacogne Jul 7, 2017

Member

still true

#endif /* HAVE_PROTOBUF */
}
void DNSProtoBufMessage::addRR(const std::string& strName, uint32_t uType, uint32_t uClass, uint32_t uTTL, const std::string& strBlob)

This comment has been minimized.

@rgacogne

rgacogne Jul 7, 2017

Member

uType and uClass should be uint16_t.

requestor = newCA(dq.remoteaddr:toString())
if requestor:isIPv4() then
requestor:truncate(24)
luasmn = newSuffixMatchNode()

This comment has been minimized.

@rgacogne

rgacogne Jul 7, 2017

Member

You have inserted a lot of trailing spaces, could you fix that?

self.assertEquals(msg.type, dnsmessage_pb2.PBDNSMessage.DNSQueryType) # testProtobuf()
else:
self.assertEquals(msg.type, dnsmessage_pb2.PBDNSMessage.DNSResponseType) # testLuaProtobuf()

This comment has been minimized.

@rgacogne

rgacogne Jul 7, 2017

Member

That blocks looks very weird because it's effectively making checkProtobufQuery() accept responses. Perhaps we should call checkProtobufResponse() when the query has been turned into a response instead?

@rgacogne

A few nits, but nothing that can't be fixed at a later time.

{
}
void add(std::string strLabel, std::string strValue)

This comment has been minimized.

@rgacogne

rgacogne Jul 17, 2017

Member

strLabel and strValue could be const references instead

std::unordered_map<std::string, std::string>tagData;
private:
const char *strSep = "\t";

This comment has been minimized.

@rgacogne

rgacogne Jul 17, 2017

Member

Could be static

#endif /* HAVE_PROTOBUF */
}
void DNSProtoBufMessage::addRR(const std::string& strName, uint16_t uType, uint16_t uClass, uint32_t uTTL, const std::string& strBlob)

This comment has been minimized.

@rgacogne

rgacogne Jul 17, 2017

Member

strName should probably be a const DNSName& instead of a string

rr->set_type(uType);
rr->set_class_(uClass);
rr->set_ttl(uTTL);
rr->set_rdata((const uint8_t *) strBlob.c_str(), strBlob.size());

This comment has been minimized.

@rgacogne

rgacogne Jul 17, 2017

Member

If I'm not mistaken the C-style cast to const uint8_t * is not needed.

@sethomisc

This comment has been minimized.

Contributor

sethomisc commented Jul 28, 2017

Remi,

changed the strLabel and strValue to be const references in Qtag::add() located in dnsdist.hh

changed const char *strSep =\t"; to static constexpr char const *strSep = "\t"; in dnsdist.hh

removed the (const uint8 t *) cast in DNSProBufMessage:addRR() in protobuf.cc

unsure if I should change strName from a string to a const DNSName& as I intended to call the function with just the DNS string like dr.qname:toString(), please advise me Remi.

Again thank you for all your input Remi, Seth

@rgacogne rgacogne added this to the dnsdist-1.2.0 milestone Jul 29, 2017

@sethomisc sethomisc force-pushed the GlobalCyberAlliance:dnsdist-mod2 branch from fbc5844 to 63801a8 Aug 1, 2017

@sethomisc

This comment has been minimized.

Contributor

sethomisc commented Aug 1, 2017

Remi,
I have rebased dnsdist-mod2.
Please let me know if there are any problems.
Seth

@rgacogne

This comment has been minimized.

Member

rgacogne commented Aug 2, 2017

Hi Seth! Thanks, there is an issue with your last commit that completely removes the README-dnsdist.md file, causing the test to file. Could you just drop that last commit and amend e75a021 ("Fixed requests from Remi July 7") to remove any alteration to that file?

sethomisc added some commits May 30, 2017

New 'clean' version of dnsdist with my modifications.
Commented line around new code segments has key words 'GCA' and 'Seth' for text searching.
Commands don't have XXX suffix anymore.
Timestamp for the setProtobufResponseType() function is supplied by lua now and not done in C++.
Current commands are: setTag, getTagArray, setTagArray, setProtobufResponseType
documentation of new lua commands, stripped down dnsdist.conf, improv…
…ed example script in pdns/zzz-gca-example directory
completed test for mods to dnsdist in pdns/regression-tests.dnsdist/t…
…est_ProtobufTag.py

script to execute it in pdns/zzz-gca-example/test-protobuf-tag.sh

@nesv nesv force-pushed the GlobalCyberAlliance:dnsdist-mod2 branch from 63801a8 to 197cc40 Aug 2, 2017

rgacogne added some commits Jul 31, 2017

dnsdist: Pass the qname as a `DNSName` to `DNSProtoBufMessage::addRR()`
It's OK for the Lua binding to accept a string, but the internal functions
should really use a `DNSName`.

@nesv nesv force-pushed the GlobalCyberAlliance:dnsdist-mod2 branch from 197cc40 to 5b8255b Aug 2, 2017

@nesv

This comment has been minimized.

nesv commented Aug 2, 2017

@rgacogne Along with your requested amendment to e75a021, I have also removed some other files that were introduced by that commit, that probably shouldn't have been there:

  • pdns/dnsdistdist/cdb.so
  • contrib/dnsmessage_pb2.py{,c}

@rgacogne rgacogne merged commit 5ab772f into PowerDNS:master Aug 3, 2017

1 check passed

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

This comment has been minimized.

Member

rgacogne commented Aug 3, 2017

Merged, thank you again!

@rgacogne rgacogne referenced this pull request Aug 3, 2017

Merged

dnsdist: Document the tag features #5577

2 of 6 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment