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

[RFC 0127] Nixpkgs "problem" infrastructure #127

Merged
merged 25 commits into from
Jul 12, 2023
Merged

Conversation

piegamesde
Copy link
Member

@piegamesde piegamesde commented Jun 11, 2022

Rendered

Implementation PR

Matrix discussion room: #nixos-rfc-127:lossy.network

Pre-RFC discussion

Discussion notice: please try to attach all discussions to a thread by using the code review feature. If your comment doesn't refer a specific line to attach to, use the header line instead.

@edolstra edolstra changed the title [RFC 0127] Nixpkgs issues and warnings (#127) [RFC 0127] Nixpkgs issues and warnings Jun 15, 2022
@edolstra edolstra added status: open for nominations Open for shepherding team nominations and removed status: new labels Jun 15, 2022
@edolstra
Copy link
Member

This RFC is now open for shepherd nominations!

@lheckemann
Copy link
Member

I nominate myself as a shepherd for this RFC.

@mweinelt
Copy link
Member

I will also nominate myself as shepherd.

@wamserma
Copy link
Member

Two remarks after having a quick glance (sorry if I missed any relevant discussion):

otherPackage.CVE1234 = "ignore"; maybe we should widen the scope of names for auto-inferred kinds and add more intelligence?

given:

meta.problems = {
 CVE-2023-XXXX = {};
 CVE-2023-YYYY = { message = "duh"; };
}

would infer:

meta.problems = {
 CVE-2023-XXXX = {
   kind = "insecure"; 
   url = "https://nvd.nist.gov/vuln/detail/CVE-2023-XXXX";
};
 CVE-2023-YYYY = { 
   kind = "insecure"; 
   url = "https://nvd.nist.gov/vuln/detail/CVE-2023-YYYY";
   message = "duh"; };
}

The other remark is re: deprecation/removal: These should take a version = "24.11" attribute, so based on nixpkgs release version we can warn on pre-deprecation and fail afterwards. And we are reminded on scheduled removal.

@7c6f434c
Copy link
Member

widen the scope of names for auto-inferred kinds

I think given the existence of auto-inference, expanding the scope should be non-controversial enough to review as the implementation becomes technically ready for inclusion? Maybe mention in future work.

@wamserma
Copy link
Member

widen the scope of names for auto-inferred kinds

I think given the existence of auto-inference, expanding the scope should be non-controversial enough to review as the implementation becomes technically ready for inclusion? Maybe mention in future work.

Definitely. We should just be careful to keep that path open in the RFC.

@mweinelt
Copy link
Member

mweinelt commented Jun 20, 2023

Linking to NVD is not the best URL I could imagine. When it comes to security issues, advisories are often a far better understandable resource, than a central database that advertises CVSS scores, which are often so-so.

Hence, I don't think auto-referencing NVD is really important here.

@7c6f434c
Copy link
Member

Linking to NVD is not the best URL I could imagine. When it comes to security issues, advisories are often a far better understandable resource, than a central database that advertises CVSS scores, which are often so-so.

As NVD usually has more or less reasonable summary and links to better write-ups, I think auto-linking NVD when there is no better link provided is useful (and fetching their links is impure so can't be done within Nix).

@RaitoBezarius
Copy link
Member

Linking to NVD is not the best URL I could imagine. When it comes to security issues, advisories are often a far better understandable resource, than a central database that advertises CVSS scores, which are often so-so.

As NVD usually has more or less reasonable summary and links to better write-ups, I think auto-linking NVD when there is no better link provided is useful (and fetching their links is impure so can't be done within Nix).

I feel like this is security team's call to decide what to do on this and is tangent to this RFC though.
In some future, we could have our own security tracker and those would be naturally the goto URLs.

@piegamesde
Copy link
Member Author

The url field is an array for a reason. We can link to NVD and advisories and Nixpkgs-specific issues all at once.

About generating those automatically, I don't know. I'm not a fan of doing some fuzzy matching on inputs in general, and also is pasting in a link every blue moon really so much effort that it is worth automating away? (https://xkcd.com/1205/ says no)

@wamserma
Copy link
Member

Linking to NVD is not the best URL I could imagine. When it comes to security issues, advisories are often a far better understandable resource, than a central database that advertises CVSS scores, which are often so-so.

I agree with you on the quality statement. :)
My intention was that there should be at least a pointer to further resources when no URL is given.

@kevincox kevincox added status: FCP in Final Comment Period and removed status: in discussion labels Jun 26, 2023
@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/tweag-nix-dev-update-50/29793/1

@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/tweag-nix-dev-update-51/30870/1

@piegamesde piegamesde deleted the rfc-127 branch August 14, 2023 10:08
@infinisil infinisil added status: accepted and removed status: FCP in Final Comment Period labels Aug 23, 2023
@Atemu Atemu mentioned this pull request Feb 28, 2024
13 tasks
KAction pushed a commit to KAction/rfcs that referenced this pull request Apr 13, 2024
* [RFC 0127] Nixpkgs issues and warnings (NixOS#127)

* Add URLs as structured information

* Update rfcs/0127-issues-warnings.md

Co-authored-by: Linus Heckemann <[email protected]>

* Update rfcs/0127-issues-warnings.md

Co-authored-by: Linus Heckemann <[email protected]>

* Update rfcs/0127-issues-warnings.md

Co-authored-by: Linus Heckemann <[email protected]>

* Update rfcs/0127-issues-warnings.md

Co-authored-by: Linus Heckemann <[email protected]>

* Update rfcs/0127-issues-warnings.md

Co-authored-by: Linus Heckemann <[email protected]>

* Update rfcs/0127-issues-warnings.md

* RFC 127 update shepherds

* RFC 127 rework

* Point out that the previous warnings system was not documented

* Rework ignore mechanism

* Update rfcs/0127-issues-warnings.md

Co-authored-by: Silvan Mosberger <[email protected]>

* Update rfcs/0127-issues-warnings.md

Co-authored-by: Silvan Mosberger <[email protected]>

* Remove "resolved" attribute again

* Incorporate review feedback

* Rewrite (again)

* Rename throw->error, trace->warn

* Make meta.problems an attrset

* Rewrite *again*, most change is in the configuration options

* Review update (WIP)

* Review update

* Update shepherds list

* Meeting update

* Typos

---------

Co-authored-by: Linus Heckemann <[email protected]>
Co-authored-by: Silvan Mosberger <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet