implementing #10 #77
Merged
implementing #10 #77
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Signed-off-by: Michael Baentsch <[email protected]>
Signed-off-by: Michael Baentsch <[email protected]>
Signed-off-by: Michael Baentsch <[email protected]>
baentsch
requested review from
bhess,
vsoftco,
dstebila,
christianpaquin,
ashman-p,
brian-jarvis-aws and
a user
|
As I didn't hear adamant opposition to this proposal in the TSC meeting, moving to Ready for Review |
SWilson4
reviewed
Sep 10, 2024
Co-authored-by: Spencer Wilson <[email protected]> Signed-off-by: Michael Baentsch <[email protected]>
dstebila
approved these changes
Sep 16, 2024
Signed-off-by: Michael Baentsch <[email protected]>
Validation failed
|
|
As expected, I just don't understand this file structure/rules. @SWilson4 Would you mind taking a second look/improve on this as per your kind offer? @open-quantum-safe/tsc Do not review (I don't seem to have permission to revert to Draft....). Sorry for the message spam. |
Signed-off-by: Michael Baentsch <[email protected]>
Validation failed
|
Signed-off-by: Michael Baentsch <[email protected]>
Validation failed
|
|
@baentsch teams cannot contain teams; that's the error |
Thanks for the hint, @ryjones So this tool has no way to express hierarchies? Every individual team (membership) must be manually set and curated? Then what's the advantage of this tool? I'd be faster clicking through GH UIs than tediously editing things this way. Would there be an alternative way to split/parcel out responsibility of the whole file somehow instead? Say OQS-global stuff here, liboqs permissions there, and so on for/within each sub project? |
|
One advantage is that there is a public audit log of changes, and as a "dsc" file (desired state configuration), if there is a change, it will be reverted. |
OK, the "revert inadvertent changes" feature sounds sensible. But does this offset months of running with a largely wrong and even insecure configuration, or making people waste hours and days having to work around problems created this way? Is it possible process is ruling safety, security, economics and common sense here? Do you know of any other (better, safer) way to achieve this, @ryjones ? Back to my question: We have to really add individual GH ids repeatedly to each and every team? If so, I have strong doubts that this can be maintained in the long run in a safe and reliable manner; particularly if more people would join the project and gaining (or losing) "cred" and thus permissions, this becomes totally unmanageable. Already the current state of things kind-of confirms this: For example, people completely external to the project have been given admin access to I assume you are most likely just following orders -- so could you please point to some decision document/meeting that agreed using this tool and procedure, @ryjones so we could discuss better, more safe and more scalable alternatives going forward in that venue? Was that the TAC? The only alternative I see is PQCA hires a dedicated security admin for the project, intimately familiar with who does what -- but honestly, I'd prefer spending money on developers and testers. Any objections to that statement @dstebila ? |
|
For now, I'll take a stab at bringing the file in line with GOVERNANCE documents and discussion in issue #10. Just wanted to check with @thb-sb: if I recall, you said in the most recent meeting that you were no longer working at Sandbox. Does that mean that we should now be using a different GitHub handle for you in the config? |
Signed-off-by: Spencer Wilson <[email protected]>
|
OK, I've taken a stab at redoing this configuration. Please take a look @open-quantum-safe/tsc. Here's a plain-English explanation of what I tried to do with the config file. If people think this analysis is valuable, I can also write it up and include it as a Markdown file for this repo. Please note that the distinction I make between "organization-level" and "repository-level" teams is one of organization. There is no functional difference in how GitHub views the teams. Organization-level teamsThese are teams which are not tied to a specific subproject and will generally have permissions across all repos in the organization. tscThe OQS Technical Steering Committee. This team should have oqs-adminsFor general GitHub administrators. Think of this as the "sysadmin" team. This team should have botsFor accounts that we use to automate actions (e.g., uploading Docker images or triggering CI jobs across projects). This team should have triageFor people who need to do issue management but are not Contributors. This team should have trail-of-bitsCreated solely to allow Trail of Bits members access to a project board tracking issues from the code audit. We can probably drop this team once the final report is released. This team doesn't need any permissions; it just pulls the ToB people into the organization. Repository-level teamsEach technical subproject has the following associated teams:
Wherever possible, I followed a GOVERNANCE.md file when defining a team. When a GOVERNANCE.md file was not available or did not specify a team, I first made a best effort to encode "folklore" (for example, setting @vsoftco as the maintainer of liboqs-go, liboqs-cpp, and liboqs-python). To handle the cases when Maintainers / Committers / etc. were not clearly specified or agreed on by unwritten consensus, I created what I consider to be sane default teams: oqs-maintainers, oqs-release-managers, oqs-committers, and oqs-contributors. oqs-maintainersConsists of TSC members who are also maintainers of an active subproject. oqs-release managersConsists of the above team plus "full-time" OQSers (i.e., myself and @praveksharma). oqs-committersConsists of the previous "core" GitHub team. oqs-contributorsPeople who have made significant code contributions and/or are listed in a CODEOWNERS file for some repository. Please take a look at this team in particular and see if I'm missing anybody. Corner casesopensslThis repo is inactive and probably should be archived. I gave it what I considered to be the bare minimum set of teams (oqs-admin and tsc). liboqs-cupqcThis repo is private and is doing its own thing. minute-takersThis team consists of people who are responsible for sharing minutes from OQS status calls (currently, me). It has write access to the tsc repo so that minutes can be uploaded without code review. |
Signed-off-by: Spencer Wilson <[email protected]>
Validation succeeded✅ The proposed configuration changes are valid!Configuration changesDirectory
Github
🔸 Please review the changes detected as they will be applied immediately once this PR is merged 🔸 |
This was referenced Sep 25, 2024
Thanks @SWilson4 for putting in this work. This tool is unusable and I don't know how one can stay sane writing the yml (or read/understand it). Kudos. So I only did spot checks -- and they're not quite satisfactory:
The last line item --plus looking beyond the review/logical perspective and just looking at whether this finally returns the privileges to where they have been at the beginning of the year-- lets me approve of this. But as author I cannot do that formally :) So a second member of @open-quantum-safe/tsc please review and approve so this can get merged. Should you get a headache trying to do this, please consider supporting my motion to replace this tool with something that actually adds assurance that code access&control privileges are correct. I just cannot believe this is used to manage large projects. |
I tried to construct the subproject-specific "release managers" teams based on who'd done past releases. The only subprojects without the default list (oqs-release-managers) are
If it's preferable to use the default list across all projects, I can make that switch easily, but I thought the exceptions made sense (especially for liboqs-rust).
I agree absolutely. Ideally we would draw this information in GOVERNANCE files (I think?). In the absence of that I opted for the global list as choosing an ad-hoc list of contributors for each subproject seemed like an ordeal. :/
I think the usage of this config file actually does help to address key person risk—someone without any admin privileges whatsoever (such as myself) can propose config changes (e.g., upgrading somebody admin privileges), and they will be made upon merge. As long as there are enough people who can approve PRs in this repo (i.e., the TSC), then I believe that we could very quickly delegate emergency admin powers to somebody in the event that a key person (say @ryjones as the universal admin) goes AWOL. We would just need to make a PR adding one line to this file (to add somebody to the oqs-admins team) and get it merged. (I should ask @ryjones to fact-check the above statement just in case I'm misunderstanding the power of this file)
Thank you! :) |
|
You might notice @thelinuxfoundation is also an enterprise owner, and therefore an org owner. There are about a dozen people that can use that account to bootstrap into an org with no other owners; they are all Linux Foundation employees. |
Signed-off-by: Ry Jones <[email protected]>
Validation succeeded✅ The proposed configuration changes are valid!Configuration changesDirectory
Github
🔸 Please review the changes detected as they will be applied immediately once this PR is merged 🔸 |
Yes, this also strikes me as casting the net for people with write access a bit too wide. In some of our GOVERNANCE.md files we distinguish between committers as having write access and contributors as not having write access, and I think that's a good distinction to maintain.
I'm less concerned about key person risk from a "lock-out" perspective -- the LF ownership of the org will ensure that someone continues to have access to the repository. I think it is a legitimate concern that we don't have enough people in certain roles (e.g., not enough people taking on maintainer roles), but that's not something that can be fixed in a config file, it's about building and strengthening the community. |
dstebila
reviewed
Sep 25, 2024
…lege * downgrade oqs-contributors to triage * merge oqs-contributors team with triage team * factor out liboqs-codeowners and oqs-provider-codeowners Signed-off-by: Spencer Wilson <[email protected]>
Validation succeeded✅ The proposed configuration changes are valid!Configuration changesDirectory
Github
🔸 Please review the changes detected as they will be applied immediately once this PR is merged 🔸 |
|
OK, I've made another iteration to (hopefully) address the least-privilege concerns. For subprojects with CODEOWNERS files (currently only liboqs and oqs-provider), I've created a |
dstebila
approved these changes
Sep 26, 2024
Reconciliation completed✅ The reconciliation completed successfully and all changes have been applied across the services!Changes appliedGithub
|
|
I see that @ryjones just merged this, and that made me realize we should make sure we understand what one of the phrases means in the new requirements, specifically: "be approved by 2 members of the OQS TSC". Michael and I are both TSC members; he authored (implying he approves of it) and I gave an approving code review; does that suffice for this purpose? Or are we interpreting this statement to mean that there should be 2 approvals by TSC members on top of any TSC members who are authors of the PR? |
An author approving of his work leads the concept of review ad absurdum. Also the author's intention with this verbiage was to stress the fact that GH permissions control has more impact on the security of the overall system's code than the number of reviews on a single PR (where |
thomwiggers
reviewed
Sep 30, 2024
5 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #10 as per #10 (comment)
Draft only pending TSC agreement