Skip to content

Comments

docs: add DNSSEC experimental-status notice#28386

Closed
Winterhuman wants to merge 4 commits intosystemd:mainfrom
Winterhuman:dnssec-experimental
Closed

docs: add DNSSEC experimental-status notice#28386
Winterhuman wants to merge 4 commits intosystemd:mainfrom
Winterhuman:dnssec-experimental

Conversation

@Winterhuman
Copy link
Contributor

@Winterhuman Winterhuman commented Jul 13, 2023

Inspired by #25676.

Adds a note to the DNSSEC entry in resolved.conf.xml stating that it is considered experimental, and should be used at your own risk.

(Apologies in advanced if I've messed up the formatting in anyway. And, I don't think there's a way to squash commits in GitHub web, so may need assistance there)

@github-actions
Copy link

An -rc1 tag has been created and a release is being prepared, so please note that PRs introducing new features and APIs will be held back until the new version has been released.

@Winterhuman Winterhuman changed the title Add DNSSEC experimental-status notice docs: add DNSSEC experimental-status notice Jul 13, 2023
@github-actions
Copy link

An -rc1 tag has been created and a release is being prepared, so please note that PRs introducing new features and APIs will be held back until the new version has been released.

@github-actions
Copy link

An -rc1 tag has been created and a release is being prepared, so please note that PRs introducing new features and APIs will be held back until the new version has been released.

@bluca
Copy link
Member

bluca commented Jul 13, 2023

I don't think this is very useful to have. Instead, if you know of things that need to be fixed, please send a patch that does that directly.

@bluca bluca added needs-discussion 🤔 and removed please-review PR is ready for (re-)review by a maintainer labels Jul 13, 2023
@Winterhuman
Copy link
Contributor Author

Winterhuman commented Jul 13, 2023

The DNSSEC feature has quite a few open issues beside #25676, and as #25676 (comment) notes it's apparently not common knowledge among users that it's experimental. I don't think think there's a way to quickly and directly fix it other than adding this doc warning

@Winterhuman
Copy link
Contributor Author

With systemd v254 almost released, I'd much rather there be at least this note for users to discover; we shouldn't let users be able to use features without them knowing any potential dangers. As @poettering mentions, DNSSEC stuff is on the backburner for now while other DNS issues are handled, so it'd be good to get this notice in as early as possible

Co-authored-by: Peter Thomassen <[email protected]>
@github-actions github-actions bot added the please-review PR is ready for (re-)review by a maintainer label Jul 14, 2023
@bluca bluca removed the please-review PR is ready for (re-)review by a maintainer label Jul 14, 2023
@bluca
Copy link
Member

bluca commented Jul 14, 2023

Again, this is not really useful, these pages get cached and stuck in various distributions and would convey wrong information forever. If you are aware of things that need fixing, just send the fixes please, thank you.

@Winterhuman
Copy link
Contributor Author

Ah, I see (not sure how these docs get stuck, since it makes me wonder how updates to documentation happen at all in that case, but I'll trust your word on that). Maybe this would be better as a temporary warning logged to journal by resolved, so once everything's fixed the logging can simply be removed without affecting documentation.

There needs to be a way to tell users about this in some way, whatever that may be.

@Winterhuman
Copy link
Contributor Author

If that is the route this goes, I can't really help there; documentation is my limit, I may need to leave that to @peterthomassen

@bluca
Copy link
Member

bluca commented Jul 14, 2023

The Github bug tracker is the appropriate place to track and describe bugs, we don't generally duplicate them in the journal or elsewhere in the codebase

@Winterhuman
Copy link
Contributor Author

Winterhuman commented Jul 14, 2023

Maybe a News/release note could work? Albeit, it'd be weird since the issue has technically been around for a while now. I'll close this pull for now; there's gotta be a better way of doing this.

How come a59703d doesn't cause caching issues btw? Is it like specific docs?

@peterthomassen
Copy link

Again, this is not really useful, these pages get cached and stuck in various distributions and would convey wrong information forever.

Wouldn't one think that docs get stuck with the same revision as the code, because they're distributed together? How come one gets stuck more than the other? (I'm really curious how that happens.)

If you are aware of things that need fixing, just send the fixes please, thank you.

The problem is that the vulnerabilities have been known for years, and there do not seem to be any resources (or a lack of knowledge) to actually fix them. Instead of letting them sit silently, it seems better to be straight about it and acknowledge the issue.

Also, in case stuff gets fixed eventually and the docs warning really would linger longer, that would hurt less than it does now hurt when you're not aware of the "experimental" status of the feature. -- If you want to cater for the case that the bugs are resolved, the warning might include a link to the relevant issues and suggest the user to check whether they're still open / apply to the release they're using. All of that is better than nothing.

In fact, the only way to figure out that the feature is "experimental" is by stumbling across the only place where that's mentioned, which is #25676 (comment). Do you expect your user base to check GitHub before relying on a feature that's documented without warning?

@pemensik fyi

@bluca
Copy link
Member

bluca commented Jul 14, 2023

Again, this is not really useful, these pages get cached and stuck in various distributions and would convey wrong information forever.

Wouldn't one think that docs get stuck with the same revision as the code, because they're distributed together? How come one gets stuck more than the other? (I'm really curious how that happens.)

Distributions render man pages on their websites too, and those are often out of date and stay that way for years because of LTS, but get privileged by page-ranking search engines because in general a popular distro website sees more links and visits and refs than a project-specific one. Anyway, the manpage is not the right place to document transient bugs, a better place is the Github tracker. The best place is to send a PR with a fix.

If you are aware of things that need fixing, just send the fixes please, thank you.

The problem is that the vulnerabilities have been known for years, and there do not seem to be any resources (or a lack of knowledge) to actually fix them. Instead of letting them sit silently, it seems better to be straight about it and acknowledge the issue.

No, the best thing is to instead spend the time to work on a PR to fix any existing issue. If you do that we'll review it.

@peterthomassen
Copy link

Distributions render man pages on their websites too, and those are often out of date and stay that way for years because of LTS, but get privileged by page-ranking search engines because in general a popular distro website sees more links and visits and refs than a project-specific one.

Interesting, thanks.

Anyway, the manpage is not the right place to document transient bugs, a better place is the Github tracker. The best place is to send a PR with a fix.

The suggestion is not to document transient bugs; I agree with you that this shouldn't be done in documentation.

The suggestion is to document the status of the feature, which is claimed to be experimental without proper labeling. It's commonplace for that to be documented, and my take is that users actually expect it. Here's an example: https://man7.org/linux/man-pages/man8/cryptsetup.8.html

Aren't we otherwise sending the message that users should assume anything in systemd to be potentially experimental? It would be interesting to learn how users are expected to determine which parts aren't ...

@bluca
Copy link
Member

bluca commented Jul 14, 2023

Look, time spent arguing about documentation or in endless discussions on a bug tracker is all time that is wasted because it will not make a fix magically appear out of thin air, and it's time that could instead be more productively spent on working on a PR, so given it seems you care about this use case, why not use such time to work on an actual fix instead?

@peterthomassen
Copy link

I don't like repeating myself, but I'll make an exception:

there do not seem to be any resources (or a lack of knowledge) to actually fix them

@pemensik
Copy link
Contributor

Distributions render man pages on their websites too, and those are often out of date and stay that way for years because of LTS, but get privileged by page-ranking search engines because in general a popular distro website sees more links and visits and refs than a project-specific one. Anyway, the manpage is not the right place to document transient bugs, a better place is the Github tracker. The best place is to send a PR with a fix.

Is it okay to mention that at least in files like /etc/systemd/resolved.conf, which should not be distributed separately? This is a place where it is permanently enabled, right?

This is not a transient bug, the same behaviour is present since RHEL8 version.

The problem is that the vulnerabilities have been known for years, and there do not seem to be any resources (or a lack of knowledge) to actually fix them. Instead of letting them sit silently, it seems better to be straight about it and acknowledge the issue.

No, the best thing is to instead spend the time to work on a PR to fix any existing issue. If you do that we'll review it.

I would say the best thing is to leave DNSSEC validation to implementations, which already were/are able and willing to fix bugs in them. Stop blocking DNSSEC enabled localhost queries, just cache them and do not interfere with them. Validation in resolved should have been just optional, but it is not. I have explained what it needs to people in Red Hat, but haven't seen a single commit or PR attempting to do that. I have tried explaining it in #19227 as best as I could.

Why do you expect anyone from outside would prepare fixes after that?

@bluca
Copy link
Member

bluca commented Jul 14, 2023

Why do you expect anyone from outside would prepare fixes after that?

Because that's how open source works. See: #25676 (comment)

@Winterhuman
Copy link
Contributor Author

Winterhuman commented Jul 14, 2023

I'm gonna reopen the pull since there's clearly a bit more nuance and debate left around this topic (sorry if this is annoying, I just really want something to come out of this decision before v254 comes out).

I thought the caching issue was strange, but I realise what you mean now: man page websites that distros run are often out of date. However, I'd argue that's the distro's issue to solve, if their documentation is out of date for this notice, all of their documentation is gonna be wrong anyways; someone else not updating their docs shouldn't be a reason we can't update systemd's upstream documentation. If it's a better idea, I can add a "as of v254 and previous" clarification to try and mitigate this further.

If a59703d can be merged, I don't see a reason why this doc change can't be as well, since any docs pre-that-patch are gonna be wrong by the same reasoning.

@poettering @keszybz Any opinions on all this? If all systemd devs who work on the networking side come to a conclusion on this patch, I'll happily follow suit.

@Winterhuman Winterhuman reopened this Jul 14, 2023
@github-actions github-actions bot added the please-review PR is ready for (re-)review by a maintainer label Jul 14, 2023
@bluca
Copy link
Member

bluca commented Jul 14, 2023

If a59703d can be merged, I don't see a reason why this doc change can't be as well, since any docs pre-that-patch are gonna be wrong by the same reasoning.

I have no idea what that commit has to do with any of this? That's just intended behavior of that setting, it's not some transient bug. Again, all the time wasted arguing here or on the bug tracker could be better spent producing actual bug fixes.

@bluca bluca added needs-discussion 🤔 and removed please-review PR is ready for (re-)review by a maintainer labels Jul 14, 2023
@Winterhuman
Copy link
Contributor Author

@bluca But it can't. V254 will release soon, and there's no way DNSSEC can be patched in, like, a week, to fix even just this issue. If nothing happens, users will keep using DNSSEC and not know about these problems; people don't check for open issues before trying every documented feature, they just stop at the man pages and trust it.

@bluca
Copy link
Member

bluca commented Jul 14, 2023

and that's not any different from the other currently open 1824 issues on the tracker

@Winterhuman
Copy link
Contributor Author

As an alternative, in case updating the documentation is really really not ideal, can a caution in the release notes at least be made? (which I'll open a separate pull for)

@Winterhuman
Copy link
Contributor Author

Many of those issues aren't security related, more like memory runaway and code-doc discrepancies, I argue this is much more important as it's networking in particular

@bluca
Copy link
Member

bluca commented Jul 14, 2023

What's important to you is irrelevant to somebody else, and viceversa. The issue tracker is to track issues, the changelog is to notify about changes.

@Winterhuman
Copy link
Contributor Author

Winterhuman commented Jul 14, 2023

Honestly, I'm gonna leave this problem for systemd to solve. DNSSEC support should of been labelled as experimental a long time ago, although I understand if the feature wasn't intended to be unstable and this is just how things played out as issues were found, however, the damage has been done; I guess "producing actual bug fixes" is the only viable path now.

Also, @bluca's dismissive and frankly rude remarks have made me uninterested in pursuing this further (seriously, you didn't need to be so impatient, I really tried to have a civil conversation here). Best of luck to @pemensik & @peterthomassen!

(I'll leave the branch open for a while in case this patch is wanted in the end)

@bluca
Copy link
Member

bluca commented Jul 14, 2023

Sorry you feel that way, but please understand that telling you no, this is not right does not equate to being rude

@Winterhuman
Copy link
Contributor Author

Winterhuman commented Jul 14, 2023

Oh no, not that; I completely sympathise with your points, but when you call discussion a "waste of time" at every turn (e.g. #28386 (comment)), it gets frustrating when people are evidently trying to genuinely ask questions or gauge alternatives with you

pemensik added a commit to pemensik/systemd that referenced this pull request Jul 15, 2023
Use just config file to declare that, note in manual were refused in
PR systemd#28386.

Signed-off-by: Petr Menšík <[email protected]>
@Winterhuman Winterhuman deleted the dnssec-experimental branch July 19, 2023 21:47
@ghost
Copy link

ghost commented Oct 28, 2023

systemd-resolved user here. I was made aware of the little-known experimental status of DNSSEC support in systemd-resolved this week. As you can read in my vulnerability report regarding DNSSEC cache handling in an older version of systemd, I was completely unaware of the experimental status and was ready to rely on systemd-resolved for validating SSHFP records, which are used at my place of work to secure SSH sessions. I would never have taken this risk if the manpage describing the DNSSEC= option mentioned that it was experimental.

Experimental software that people may rely on for security should be clearly labeled as such. Please reconsider updating the man pages with this information.

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

Development

Successfully merging this pull request may close these issues.

4 participants