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

Updated cookie secrets management #378

Merged
merged 11 commits into from
Oct 23, 2024
Merged

Updated cookie secrets management #378

merged 11 commits into from
Oct 23, 2024

Conversation

wtoorop
Copy link
Member

@wtoorop wtoorop commented Aug 28, 2024

This PR is in response to issue #196 (and thus also of interest to @anandb-ripencc )

  • The cookie secret file's default location can be set with --with-cookiesecretsfile=path with configure
  • The default cookie secrets file is changed to {dbdir}/cookiesecrets.txt
  • When no cookie secrets file location is set with the cookie-secret-file option in the config file, and the new default location does not exist, cookie secrets will be read from the previous default location {configdir}/nsd_cookiesecrets.txt
  • A staging cookie secret can also be provided in the config file with the cookie-staging-secret option.
  • Explicitly configured cookie secrets in config file (with the cookie-secret and cookie-staging-secret option) take precedence over the ones from the cookie secret file
  • All values of DNS cookie related settings from config file (answer-cookie, cookie-secret, cookie-staging-secret and cookie-secret-file) will be reevaluated and effectuated after nsd-control reconfig

from the old to the new default location
even when they came from the old default location
+some small code cleanup and reduced code repetition
@wtoorop
Copy link
Member Author

wtoorop commented Aug 29, 2024

It's ready for review (and merge if accepted). One thing to consider/discuss is that it has one thing that is not backwards compatible, which is that cookie secrets from config file now are preferred over cookies from the cookie secrets file. Before this was the other way around. But, this feels more logical to me as zones in the configuration file are also preferred over zones in the zone list file. I believe there will be a negligible number of people that have cookie secrets in both the config file and in the cookie secrets file and would depend on the earlier preference to pick the secrets file.

@anandb-ripencc
Copy link
Contributor

Looks reasonable to me. I agree that despite the backwards-incompatible change, it is unlikely to affect anyone, because no-one would define cookies in both places.

Copy link
Member

@wcawijngaards wcawijngaards left a comment

Choose a reason for hiding this comment

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

The code looks all right. I saw some places where the cookie secrets could possibly be wiped after temporary variables are done being used, maybe that is nice to have.

difffile.c Show resolved Hide resolved
remote.c Show resolved Hide resolved
remote.c Show resolved Hide resolved
remote.c Show resolved Hide resolved
util.c Outdated Show resolved Hide resolved
util.c Outdated Show resolved Hide resolved
util.c Outdated Show resolved Hide resolved
util.c Show resolved Hide resolved
Thanks @wcawijngaards ! Yes, cleanup secrets on the stack after use is definitely more secure. I think you even found some occurrences that weren't covered before.

Co-authored-by: Wouter Wijngaards <[email protected]>
Copy link
Contributor

@k0ekk0ek k0ekk0ek left a comment

Choose a reason for hiding this comment

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

I agree with the changes in general, just some small remarks regarding use of enums and one related to falling back to the previous value.

Comment on lines +2563 to +2565
default:
ssl_printf(ssl, "source : unknown\n");
break;
Copy link
Contributor

Choose a reason for hiding this comment

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

All possibilities are covered, so this is a panic situation(?)

Copy link
Member Author

Choose a reason for hiding this comment

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

I guess yes. Would you prefer a assert here?

Copy link
Member Author

Choose a reason for hiding this comment

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

Would you prefer an assert?

util.c Outdated Show resolved Hide resolved
util.c Outdated Show resolved Hide resolved
util.c Outdated Show resolved Hide resolved
@anandb-ripencc
Copy link
Contributor

I was reviewing the changes to the man pages and documentation, and it needs spelling and syntax fixes. Let me comment on the changes so you can fix them before merging this branch.

@wtoorop
Copy link
Member Author

wtoorop commented Oct 22, 2024

I was reviewing the changes to the man pages and documentation, and it needs spelling and syntax fixes. Let me comment on the changes so you can fix them before merging this branch.

Thanks! I'm also still addressing the last comment from @k0ekk0ek, which will simplify the code somewhat and also only does the fallback to the old default location for secrets (if there is nothing configured and nothing in the new default location), when nsd just started.

@wtoorop
Copy link
Member Author

wtoorop commented Oct 23, 2024

I was reviewing the changes to the man pages and documentation, and it needs spelling and syntax fixes. Let me comment on the changes so you can fix them before merging this branch.

@anandb-ripencc I am done with review feedback and will merge once you're finished your spelling corrections.

@anandb-ripencc
Copy link
Contributor

Actually, just merge it. I will then submit a PR to update the documentation. Please don't make a new release before I've submitted documentation fixes.

@k0ekk0ek
Copy link
Contributor

Looks good te me @wtoorop 👍

@wtoorop
Copy link
Member Author

wtoorop commented Oct 23, 2024

I accidentally committed along changes in simdzone. I'll force push the commit without the simdzone changes, and then merge.

Right after config is read, so no wrapper is needed anymore to determine the value.
@wtoorop wtoorop merged commit 40c23b1 into master Oct 23, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants