Design document for adding roles to manual judgment stage. #80
Conversation
sanopsmx
requested review from
ajordens,
ethanfrogers,
ezimanyi,
plumpy and
robzienert
as code owners
|
This RFC is linked to a previous RFC where initial feedback was given - #78 |
|
Implementation-wise, I'd suggest starting with the restrictions in The fact that continue/cancel buttons are hidden is definitely a usability improvement but the important checks happen in As an aside, allowing for free-form group specification (vs only selecting from a list) seems reasonable to me but an implementation concern. It would certainly be an improvement over Overall, I'm 👍 to the idea and see it as an incremental improvement over |
|
We did not hear any feedback from security SIG. Checks happen in both UI and orca. |
|
@bpowell @helenelau - Sounds like this RFC is currently in your court. |
|
@bpowell let's discuss |
|
@sanopsmx can you attend the next security sig meeting on 03/02? |
|
Yes will attend the meeting on 03/02.
Regards,
Sanjeev
…On Thu, 27 Feb 2020 at 02:01, Helene Lau ***@***.***> wrote:
@sanopsmx <https://github.com/sanopsmx> can you attend the next security
sig meeting on 03/02?
https://github.com/spinnaker/governance/tree/master/sig-security
https://docs.google.com/document/d/1GhzhmG7ZytJBfW2lontOSBxVghgNZmxstF49SoocTDQ/edit#heading=h.ilm7rdxf4d42
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#80?email_source=notifications&email_token=AM4NAFFYBPTHTV7THHEKJZ3RE3GR7A5CNFSM4J2CRKO2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOENBYP5I#issuecomment-591628277>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AM4NAFCGL23Y2P3X32SAJALRE3GR7ANCNFSM4J2CRKOQ>
.
|
|
FYI, the security SIG meeting is actually on 3/5. |
|
Yes thanks for catching it. @sanopsmx 03/05 it is. |
|
Will attend on 03/05. Can you please let us know the time and bridge
details.
…On Mon, 2 Mar 2020 at 23:45, Helene Lau ***@***.***> wrote:
Yes thanks for catching it. @sanopsmx <https://github.com/sanopsmx> 03/05
it is.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#80?email_source=notifications&email_token=AM4NAFH3SHOWPMVGWAIRL7LRFPZUZA5CNFSM4J2CRKO2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOENQLVFI#issuecomment-593541781>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AM4NAFBXBLLGRKB562SEFSTRFPZUZANCNFSM4J2CRKOQ>
.
|
|
The info is at the links in my previous comment. In particular, it’s at
the top of the google doc.
|
|
Hi all. I've (internally) been working on a similar proposal, which has more of a higher-level goal discussion and less technical details - would be nice if I could get some feedback on this, and maybe somebody could work to combine the two proposals or something. https://docs.google.com/document/d/15zlA80xCrkCn4L08YbrBDDPJsL--9lUlNeWooZy4Nk8/edit?usp=sharing= |
|
@justinrlee please join our meeting at 11am pt ... meet.google.com/rhf-kyvh-dre @sanopsmx fyi |
|
Hey @helenelau I was actually hoping to join but I have another prior engagement at that time. One of my coworkers will be joining to represent in my stead. |
|
@helenelau Is there any update from the prior Security SIG here? Was this RFC discussed at all? |
|
discussed ...Are we having the sig meeting again on march 19th....
…On Wed, 18 Mar 2020 at 01:30, Rob Zienert ***@***.***> wrote:
@helenelau <https://github.com/helenelau> Is there any update from the
prior Security SIG here? Was this RFC discussed at all?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#80 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AM4NAFBYR4IM3VJ2S2IDF4DRH7JGZANCNFSM4J2CRKOQ>
.
|
|
@sanopsmx yes tomorrow at 11am pt ... @justinrlee fyi |
|
@sanopsmx It looks like you moved the RFC template. Can you fix the PR so that the template is still there? |
|
Raised a new pull request |
|
created a new PR |
|
reopening this PR. Will update the PR. |
|
Updated the comments...Modified few testcases...incorporated the sig meeting feedback.. |
|
@bcornils fyi |
|
Can you share the meeting invite. Is the meeting scheduled on Julu 23rd. |
|
Yes. This Thursday at 11 pdt. The link is in the security sig slack channel.
…Sent from my iPhone
On Jul 21, 2020, at 9:56 PM, Sanjeev Thatiparthi ***@***.***> wrote:
Can you share the meeting invite. Is the meeting scheduled on Julu 23rd.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or unsubscribe.
|
|
@maggieneterval ...I gave the presentation to the sig meeting on July 23rd. Incorporated all the comments suggested in the earlier march meeting. I think, only merge is pending. |
|
Thanks for the update, @sanopsmx. Should we update this RFC as accepted and change the SIG designation to Security? @helenelau @bcornils I've reviewed the Deck component of this change, but I'll leave it to y'all to identify an appropriate reviewer for the Orca component. |
rfc/.manualjudgment.md
Outdated
| | **Status** | _**Proposed**, Accepted, Implemented, Obsolete_ | | ||
| | **RFC #** | https://github.com/spinnaker/deck/pull/8368 | https://github.com/spinnaker/orca/pull/3763 | ||
| | **Author(s)** | Sanjeev Thatiparthi (@sanopsmx) (https://github.com/sanopsmx) | ||
| | **SIG / WG** | sig-ux |
|
I like the idea of this! I think the Orca PR needs some improvement, but I can give that feedback in the actual PR. One thing I did notice that is relevant to call out here - the PR checks for isTest, which is not noted in this design doc. At first glance, I don't like pushing that logic (checking if an environment is a test environment or not) into a manual judgement stage, but I could be convinced otherwise. |
Removed 'isTest' flag according to the comments mentioned. |
|
Can you review this PR and merge. Accommodated all the suggested changes in the sig-meeting. |
|
@sanopsmx I will merge as soon as you un-delete the RFC template file and rename the RFC to |
|
Renamed the .manualJudgment.md to manualJudgment.md |
Renamed the .manualJudgment.md to manualJudgment.md |
Renamed the .manualJudgment.md to manualJudgment.md |
Select roles that can execute manual judgement stage
Overview
Display all the roles of the user added in the application in the manual judgment stage.If there are 2 groups(dev,qa)
and qa group has the permission to deploy.If there is a manual judgment stage before the deploy stage.If there are
no groups attached to the manual judgment stage, then dev group can continue with the manual judgment stage and get
failed at the deploy stage as only qa group has the permission to deploy. This is the reason we are displaying all
the roles of the application. This is a usability feature.
Goals and Non-Goals
Display authorized groups dropdown list only when the user selects the manual judgment stage.
Display all the groups the user has selected while creating application(permissions) in the authorized groups dropdown.
When the pipeline is executed, disable the manual judgment 'continue/cancel' based on the user role selected in the authorized groups.
opsmxemp1 : ["mail-qa","sra-dev","mail-dev","sra-qa","empdev"]
User can opt out of this feature as stated in the testcase 2 & 3.
If authz is not enabled, it will work in the same way.
The above test cases will handle all the test scenarios.
If the role is set in the manual judgment stage, then we check the stage role both in the application roles and user roles.
If yes,we check for application permissions of that role. If yes(stage role has execute/write/create application permission),
we can approve and move to the downstream stage, if no(stage role has read application permission), then we deny
(disable the continue button in the manual judgment stage).If the pipeline is triggered from command line,
then the backend orca code checks the same above mentioned logic.If yes,we approve and moves to the
downstream stage , if no, then we deny(It will be running in the same stage).
Motivation and Rationale
Only legitimate users can approve the manual judgment stage so that it can be moved to downstream stages.
Timeline
Already Implemented
Design
Enhanced stage.html to
Get the roles of the application.
Display the roles of the application as a list only if the stage is Manual Judgment.
Enhanced stage.module.js to
Fetch the permissions of the application from the gate application url.
Application permissions has all the roles.
Populate the list with only the roles of the application with no duplicates.
Enhanced ApplicationReader.ts to
Fetch the permissions of the application from the gate application url.
Fetch the roles of the application, stage and the user during the execution.
Iterate through each of the user role to check if the role exists in the stage and application.
We also check for stage role application permission.
If the stage role has execute/write/create application permission, then enable the continue button.
If the stage role has read application permission, then disable the continue button.
Enhanced OperationsController.groovy to
Get the application roles, stage roles and the user roles.
Check each of the user roles whether they are contained in the stage and application roles.
We also check for stage role application permission.
If the stage role has execute/write/create application permission, then we set the isAuthorized flag to true.
If the stage role has read application permission, then we set the isAuthorized flag to false.
By default, all the stages except Manual Judgment are true.
Enhanced ManualJudgmentStage.groovy to
Check for the flag in ManualJudgmentStage.groovy whether to execute to the next stage or not.
If yes/no, continue with the next stages/continues running the same stage.
Enhanced ManualJudgmentStageSpec.groovy to
Modified the testcases as per the new logic.