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

dnsdist: add ability to update webserver credentials #7117

Merged
merged 4 commits into from Oct 31, 2018

Conversation

chbruyand
Copy link
Member

Short description

This PR adds setWebserverConfig lua funciton to update webserver credentials and custom headers.

Fix #7112

Checklist

I have:

  • read the CONTRIBUTING.md document
  • compiled this code
  • tested this code
  • included documentation (including possible behaviour changes)
  • documented the code
  • added or modified regression test(s)
  • added or modified unit test(s)

@@ -631,7 +631,8 @@ void setupLuaConfig(bool client)
SBind(sock, local);
SListen(sock, 5);
auto launch=[sock, local, password, apiKey, customHeaders]() {
thread t(dnsdistWebserverThread, sock, local, password, apiKey ? *apiKey : "", customHeaders);
setWebserverConfig(password, apiKey, customHeaders);
thread t(dnsdistWebserverThread, sock, local);
t.detach();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a nit but indentation seems off.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The file has been partially indented with tabs :/

@rgacogne rgacogne added this to the dnsdist-1.3.x milestone Oct 30, 2018
@@ -446,6 +446,7 @@ const std::vector<ConsoleKeyword> g_consoleKeywords{
{ "setUDPMultipleMessagesVectorSize", true, "n", "set the size of the vector passed to recvmmsg() to receive UDP messages. Default to 1 which means that the feature is disabled and recvmsg() is used instead" },
{ "setUDPTimeout", true, "n", "set the maximum time dnsdist will wait for a response from a backend over UDP, in seconds" },
{ "setVerboseHealthChecks", true, "bool", "set whether health check errors will be logged" },
{ "setWebserverConfig", true, "password [, apiKey [, customHeaders ]]", "Updates webserver configuration" },
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps this function should take a table? A user could then update one of the items without updating the others. e.g.

setWebserverConfig({password: "hello"})
setWebserverConfig({apiKey: "hello2"})

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this means using setWebserverConfig({apiKey: ""}) to disable apiKey authentication. Is that something we want ? Otherwise I agree.

@chbruyand
Copy link
Member Author

I followed pieter's remark and added two basic tests about custom headers

setWebserverPassword(password);
}
if(vars->count("apiKey")) {
// allows setting apiKey: nil to disable access with it
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does nil really work? It looks like we only test for an empty string?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

indeed

@rgacogne rgacogne merged commit 2aa5488 into PowerDNS:master Oct 31, 2018
@chbruyand chbruyand deleted the dnsdist-webserver-creds branch October 31, 2018 09:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants