project: Pin Python dependencies

[pdgendt]

Generate pinned requirement files from pyproject.toml for tox.

Additionally create pinned requirement files for the Github workflows.

[marc-hb]

first impressions

[marc-hb]

Can colorama really affect release output? That sounds like serious security design attack surface issues if it can.

[pdgendt]

Not sure what you mean with this comment, colorama is a dependency of tox, this file was generated with only tox as the package to install.

[marc-hb]

If colorama is a dependency of tox only, then it does not need to be pinned to a specific SHA because tox should NEVER be in a position to affect source code or releases, or have any side effect. It should simply not be part of that attack surface.

Same for this entire requirements-tox.txt file.

[marc-hb]

Any idea why some dependencies need a crazy number of hashes?

[pdgendt]

They can have different pre-built binaries for different platforms, and because of that all the hashes need to be added.

[pdgendt]

Every distribution of a package (specific python version, platform, output format like wheels vs tarball) needs to be added to the list of hashes.

[marc-hb]

Isn't there a way to "checksum the list of checksums" and reduce the number?

Checksums are meant to be compared, that's their only purpose. Often enough, they need to be compared by humans but no one is going to compare dozens of checksums visually.

[pdgendt]

I don't think so, not at least where other tooling (dependabot) can still parse them.

[marc-hb]

not at least where other tooling (dependabot) can still parse them.

It does not have to be mutually exclusive. We could have both the list and its sum (if there were such a sum)

[pdgendt]

Not sure what to do here, this is generated using tools I frequently use.

[marc-hb]

Why do we need the same hashes 4 times? Is there no way to include and re-use?

[pdgendt]

What do you mean with the same hashes? For the different requirements files? These are generated with the entire chain of sub-dependencies, not sure if common packages could be extracted.

[pdgendt]

Also installing all the different requirement files would simply skip already installed packages. The hashes are identical.

[marc-hb]

Thanks for your patience, I'm slowly making sense of all this.

I don't see why we need to pin tox dependencies. I hope tox is not involved in the release process, is it? I mean, I hope there is no way a compromised tox could affect the release output in any way, correct? If it were capable of that, that would be a much bigger problem.

[pdgendt]

It's one of the medium security issues checked by the OpenSSF tool: https://github.com/zephyrproject-rtos/west/security/code-scanning/26

[pdgendt]

Even in the "read-only" case we want to ensure that our simplest CI actions are running the tools we expect them to run. Be it setuptools, build, colorama, tox, ... really any tool or package.

That's just a statement without a rationale.

Apologies, this was not intentional, I guess I am unable to answer your initial question.

As @kartben mentioned, access to CI already opens that "read-only" view. So if that's what you're referring to as "least privilege" then I guess we can't trust Github for that matter.

It's not about "trusting GitHub.com" in general, I agree there is not a lot we can do there. It's about trusting project-specific, automated workflows and controlling their respective access levels.

So are you saying that all GitHub workflows must have the same, full access level no matter what side effects they need? In other words, running pylint or creating a new, official release requires the same level of complete access? Not having something that simple and that effective would be very sad, even more sad when compared to the incredibly fine-grained docs.github.com/en/organizations/managing-peoples-access-to-your-organization-with-roles/roles-in-an-organization that no one ever fully understands and to all the incoming CodeQL, pinning and Dependabot complexity.

@marc-hb I appreciate your efforts and value your opinion, but in the context of the PR, I have no clue what to do with this comment. Is this a showstopper? Can you give any suggestions on what would be a better workflow/solution?
I'd like to move forward with this, as I think it is an improvement, both for testing and packaging. It follows best practices. It relies on tooling, yes, but alongside automation that should help maintainers.

[marc-hb]

So are you saying that all GitHub workflows must have the same, full access level no matter what side effects they need? In other words, running pylint or creating a new, official release requires the same level of complete access?

Do you know the answer?

[henrikbrixandersen]

So are you saying that all GitHub workflows must have the same, full access level no matter what side effects they need? In other words, running pylint or creating a new, official release requires the same level of complete access?

Do you know the answer?

The closest, I can find, is this: https://docs.github.com/en/actions/writing-workflows/workflow-syntax-for-github-actions#jobsjob_idpermissions

[marc-hb]

Thanks @henrikbrixandersen . So it does look indeed possible to de-privilege test runs and code scanners. Unlike rubber-stamping dozens of checksums, this looks like actual security.

[pdgendt]

Yes, this is already applied to all current workflows with:

permissions:
  contents: read

Any other permission is defaulted to none:

If you specify the access for any of these permissions, all of those that are not specified are set to none.

[marc-hb]

I'm confused by the file names: requirements-tox.txt is used in .github/ CI while requirements-ci.txt is used in tox.ini... is this intended?

Automation aside, after this PR how can I simply test the latest version of all dependencies and in my workspace and detect any new incompatibility or regression in them?

[pdgendt]

Yes, the file names maybe aren't clear, currently:
requirements-tox.txt -> the dependency chain to install tox itself
requirements-ci.txt -> the dependencies to run tox
requirements.txt -> the dependencies of west
requirements-build.txt -> the dependencies to create a package

Automation aside, after this PR how can I simply test the latest version of all dependencies and in my workspace and detect any new incompatibility or regression in them?

I was thinking about making this easier too, I can add a script that would create/update the requirements files (without dependabot).

[pdgendt]

Renamed the files, now:

requirements.txt -> the dependencies of west
requirements-tox.txt -> the dependencies to run tox
requirements-install-build.txt -> the dependencies to create a package
requirements-install-tox.txt -> the dependencies to install tox

[marc-hb]

I have no clue what to do with this comment. Is this a showstopper? Can you give any suggestions on what would be a better workflow/solution?

Let me try to clarify.

• Least privilege: this is the most important, much more than pinning. All dependencies must have the minimum possible level of access to do their job. Because of configuration complexity, this does not come for free but it is much more important than pinning dependencies and it is also much better security "value" as it reduces attacks surface completely and permanently: no routine reviews of changes needed. Looks like you just checked that?

• "If it's not tested then it does not work": because GitHub permission levels seem complex, after configuring workflow permissions "someone" should attempt to perform side effects or expose secrets and make sure access is actually denied. This should be done again every time GitHub makes some change to how permissions work (hopefully: rarely).

• Privileged dependencies that do have a need to perform any side-effect ("write" access) or that have "read" access to any secret token or any other non-public data should absolutely be pinned like this PR does. I never questioned that. I'm afraid I might simplify GitHub access levels but you should get the idea.

• The number of complexity of privileged dependencies should be reduced to an absolute minimum in the first place.

• Other, de-privileged dependencies maybe be pinned to some major version for functional reasons but they should IMHO not be micro-managed with checksums etc. because this unnecessarily and considerably increases the workload of routine updates and steals precious time from other, actually effective security tasks - like detailed and very time-consuming reviews of changes in privileged dependencies.

"Security theater" sounds harmless and even fun maybe. But it is actually a dangerous distraction because it steals scarce attention and brain cycles and because routine tasks are mind-numbing.

Speaking of scarce attention and distractions, OS images of runners offer a much larger attack surface. Are those all pinned too and who's going to review updates of entire images like Ubuntu and others? I noticed that GitHub performs "continuous" updates of its default runners.
actions/runner-images#741 (comment)
Not sure these can even be pinned?

[marc-hb]

Other, de-privileged dependencies maybe be pinned to some major version for functional reasons but they should IMHO not be micro-managed with checksums etc. because ...

Even if you disagree with this, you'll agree the risk level is much lower. So these could be done later - after learning from experience with privileged dependencies. "Smaller PRs".

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 84.06%. Comparing base (4638712) to head (5c0ac31).

Additional details and impacted files

@@           Coverage Diff           @@
##             main     #802   +/-   ##
=======================================
  Coverage   84.06%   84.06%           
=======================================
  Files          11       11           
  Lines        3345     3345           
=======================================
  Hits         2812     2812           
  Misses        533      533           

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[carlescufi]

@marc-hb @henrikbrixandersen @kartben is anything here not ready for merging? It'd be great to move this forward.

@marc-hb in particular, I am not sure by reading your comments whether you are OK with this change or not.

[marc-hb]

@marc-hb in particular, I am not sure by reading your comments whether you are OK with this change or not.

I'm OK with only "half" of it. Pinning requirements.txt makes sense, pinning requirements-tox.txt does not.

I'm very concerned about the perception that pinning is the latest security silver bullet. For years we were told to "update, update and update" to stay secure. Now it's the opposite. Which is it? Neither, it's complicated. And it can never be that "cheap".

EDIT: Pinning is only one tool in the box. It's not a hammer that sees everything as a nail.

[marc-hb]

If colorama is a dependency of tox only, then it does not need to be pinned to a specific SHA because tox should NEVER be in a position to affect source code or releases, or have any side effect. It should simply not be part of that attack surface.

Same for this entire requirements-tox.txt file.

[pdgendt]

I'm OK with only "half" of it. Pinning requirements.txt makes sense, pinning requirements-tox.txt does not.

I have to disagree here, pinning requirements-tox.txt is no different than pinning requirements.txt IMO. It both only has effect in CI. If anything, pinning requirements-tox.txt makes sure, that if a user tests locally and succeeds, it will do too in CI.
We should not burden users that are fixing bugs or introducing features in west itself, with fixing dependency issues. Those would need separate attention.

I'm very concerned about the perception that pinning is the latest security silver bullet. For years we were told to "update, update and update" to stay secure. Now it's the opposite. Which is it? Neither, it's complicated. And it can never be that "cheap".

EDIT: Pinning is only one tool in the box. It's not a hammer that sees everything as a nail.

It isn't a silver bullet, this has never been stated. It is an improvement as it pins the "current" state of what is thought to be an accepted version of the tool. I would argue we still do "update, update, update", but I prefer to offload that to tooling, and in this case dependabot.
We can better track what dependencies would cause issues as they are treated outside of the other changes.

[pdgendt]

As a side-note: west has been on Homebrew for years, a super popular package manager, primarily for macOS. It is super automated and pins EVERYTHING. It updates the dependencies in their west formula.

[marc-hb]

I have to disagree here, pinning requirements-tox.txt is no different than pinning requirements.txt IMO.

I tried to phrase the difference in 2 or 3 different ways. Maybe none were well written, but your reply does not even try to mention any of them. I give up!

[pdgendt]

I tried to phrase the difference in 2 or 3 different ways. Maybe none were well written, but your reply does not even try to mention any of them. I give up!

I'm sorry, I may have difficulties explaining this, but requirements.txt is only relevant during CI, as is requirements-tox.txt. During installation of west itself, it isn't used.

[marc-hb]

I'm sorry, I may have difficulties explaining this, but requirements.txt is only relevant during CI, as is requirements-tox.txt. During installation of west itself, it isn't used.

I think that is still besides my main point.

I would argue we still do "update, update, update", but I prefer to offload that to tooling, and in this case dependabot.

dependabot is nice but it does absolutely nothing to help with actual security audits. Except giving a bit of a false sense maybe.

[marc-hb]

It [homebrew] is super automated and pins EVERYTHING.

Off-topic but since this thread is doomed anyway...

I've used homebrew on my mac since forever. Once, I tried to make a one-line change in one of the packages. That's the "essential" litmus test. I tried for hours and gave up. Probably unrelated to pinning but really not impressed.

[carlescufi]

@marc-hb this has been now open for a while. My impression when reading your comments is that you disagree with pinning everything as a rule, but are not strictly opposed to using pinning as tool.

So here's my question: if @pdgendt only pinned requirements.txt but not requirements-tox.txt you'd be OK with this proposed change? If that's the case then I suggest @pdgendt makes a call on whether to pin only one or both. Also, if you are strictly opposed to pinning both then please NACK the PR. Thanks!

[pdgendt]

I'm not in favour of doing only one, but not the other. I'll close this and not include it in v1.4, maybe try to come up with something for v2.0.
Not a hill I want to die on.