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

[master] Add autosign_grains to auth events with action 'pend' #65426

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

thus
Copy link

@thus thus commented Oct 18, 2023

What does this PR do?

It adds the "autosign_grains" that the minion sends during auth to the auth event that the master sends. This enables us to create a runner to do more cool autosign stuff (more than just shared secrets, which is already supported by autosign_grains).

To enable it, the option auth_events_pend_autosign_grains is added. By default it is false. When enabled, it only passes on the "autosign_grains" when the action is pending. This means that it not added any more when a key is accepted, rejected or denied.

Example auth event (with autosign grains):

salt/auth       {
    "_stamp": "2023-10-18T11:57:23.212718",
    "act": "pend",
    "id": "test.example.com",
    "pub": "<REDACTED>",
    "autosign_grains": {
        "autosign_key": "abcdefgh"
    }
}

As far as I can see, people has wanted something similar in the past: #37712, #43394, #56189 (all closed issues)

In addition to this, I also fixed so all auth events have the "act" field set (#56200) and moved variables that was only used when auth events were enabled.

What issues does this PR fix or reference?

Fixes: #56200
(not the main goal of the PR, but I had to touch that part of the code anyway)

New Behavior

Add "autosign_grains" to auth events when "act" is "pend".

Merge requirements satisfied?

[NOTICE] Bug fixes or features added to Salt require tests.

Commits signed with GPG?

No

Please review Salt's Contributing Guide for best practices.

See GitHub's page on GPG signing for more information about signing commits with GPG.

@thus thus requested a review from a team as a code owner October 18, 2023 12:10
@thus thus requested review from felippeb and removed request for a team October 18, 2023 12:10
@welcome
Copy link

welcome bot commented Oct 18, 2023

Hi there! Welcome to the Salt Community! Thank you for making your first contribution. We have a lengthy process for issues and PRs. Someone from the Core Team will follow up as soon as possible. In the meantime, here’s some information that may help as you continue your Salt journey.
Please be sure to review our Code of Conduct. Also, check out some of our community resources including:

There are lots of ways to get involved in our community. Every month, there are around a dozen opportunities to meet with other contributors and the Salt Core team and collaborate in real time. The best way to keep track is by subscribing to the Salt Community Events Calendar.
If you have additional questions, email us at [email protected]. We’re glad you’ve joined our community and look forward to doing awesome things with you!

@salt-project-bot-prod-environment salt-project-bot-prod-environment bot changed the title Add autosign_grains to auth events with action 'pend' [master] Add autosign_grains to auth events with action 'pend' Oct 18, 2023
@thus
Copy link
Author

thus commented Nov 12, 2023

What is the timeline for reviewing this PR?

Please let me know if there is anything I can do to help or if there is anything missing that I should fix.

@thus
Copy link
Author

thus commented Nov 27, 2023

@felippeb: bump! :)

@max-arnold
Copy link
Contributor

max-arnold commented Dec 8, 2023

Programmatic control of what gets autoaccepted is nice!

  1. There is no need to modify the man page - it will be generated automatically
  2. Versionadded should be 3007 (hopefully)
  3. Do you see any valid use-cases for these grains being added to different auth events (not only pend)? If so, maybe the setting could be more generic: auth_events_autosign_grains: [pend, accept, reject, denied, full, error]
  4. If you have the time, please document the remaining act values here: https://docs.saltproject.io/en/latest/topics/event/master_events.html#event-master-auth

@thus
Copy link
Author

thus commented Dec 8, 2023

Thanks for commenting my PR, @max-arnold :)

  1. There is no need to modify the man page - it will be generated automatically

Thanks, I'll remove it.

  1. Versionadded should be 3007 (hopefully)

I'll update the version.

  1. Do you see any valid use-cases for these grains being added to different auth events (not only pend)? If so, maybe the setting could be more generic: auth_events_autosign_grains: [pend, accept, reject, denied, full, error]

That would make it more flexible and useable for even more use cases, so I think it's a great idea. I originally didn't want to do it like that, since I didn't want to add options for every auth event (auth_events_pend_autosign_grains, auth_events_accept_autosign_grains, and so on). I didn't think of using a list, which looks really elegant! I'll change the code to work for every auth event.

  1. If you have the time, please document the remaining act values here: https://docs.saltproject.io/en/latest/topics/event/master_events.html#event-master-auth

I'll take a look at fixing the documentation, as well :)

Thanks again for your time!

doc/ref/configuration/master.rst Outdated Show resolved Hide resolved
@thus
Copy link
Author

thus commented Dec 8, 2023

Thanks, @twangboy!

Do you want the changes in this PR or should I create a new branch, open a new PR and close this one? My experience is that different project teams have different preferences regarding this :)

@twangboy
Copy link
Contributor

twangboy commented Dec 8, 2023

Just make the changes in this branch. You'll need to pull since I rebased this PR.

@dwoz dwoz added this to the Argon v3008.0 milestone Dec 18, 2023
@thus thus force-pushed the thus_auth-events-pend-autosign-grains_v1 branch from ecd9e0c to 39328e6 Compare February 23, 2024 09:00
@thus
Copy link
Author

thus commented Feb 23, 2024

Sorry for the delay, I had some other projects I needed to finish first. I have now rebased with master, and I'm working on implementing the changes you wanted.

@thus thus force-pushed the thus_auth-events-pend-autosign-grains_v1 branch from 39328e6 to 5e2aa78 Compare February 23, 2024 15:06
@thus
Copy link
Author

thus commented Feb 23, 2024

Changes:

  • Removed the man page changes I did earlier.
  • Changed version added to 3007.
  • Added the missing actions in salt/auth events to the documentation.
  • Added another test to test for when the option is not present.
  • Modified the option to support logging autosign_grains for all the actions, by adding the actions to a list, as suggested.

Thanks for the help so far! Have a nice weekend!

@thus thus requested a review from twangboy February 23, 2024 15:16
twangboy
twangboy previously approved these changes Feb 26, 2024
Adding autosign_grains for an action is enabled by adding the action to
the 'auth_events_autosign_grains' (list) option on the master, which is
empty by default.

As an example, we could add the following to add autosign_grains to
all auth events with action 'pend' (approval pending):
  auth_events_autosign_grains: ["pend"]
@thus thus force-pushed the thus_auth-events-pend-autosign-grains_v1 branch from 5e2aa78 to 2c6fa20 Compare April 28, 2024 20:10
@thus
Copy link
Author

thus commented Apr 28, 2024

Bumped the version added to 3008, and fixed a typo I made in the documentation :)

@thus
Copy link
Author

thus commented May 6, 2024

Any chance of getting a new review? 😄

@thus
Copy link
Author

thus commented May 13, 2024

@twangboy: I hit the wrong button, sorry!

@thus
Copy link
Author

thus commented Jul 25, 2024

Any chance of getting this merged soon? 😊

@thus
Copy link
Author

thus commented Sep 3, 2024

@twangboy: bump! :)

Copy link
Contributor

@twangboy twangboy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would you mind looking at the conflict?

@tazaki
Copy link

tazaki commented Oct 18, 2024

@thus if you have time to fix that would be great!

@thus thus requested a review from a team as a code owner October 18, 2024 07:53
@thus
Copy link
Author

thus commented Oct 18, 2024

@twangboy, @tazaki: The conflict was due to a trivial and non-related change. It is resolved now :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

salt/auth 'act' field is missing
5 participants