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

Add SCRAM-SHA-256 support to postgres states #59034

Merged
merged 18 commits into from Mar 1, 2021

Conversation

OrangeDog
Copy link
Collaborator

@OrangeDog OrangeDog commented Nov 27, 2020

What does this PR do?

Adds SCRAM-SHA-256 support to postgres states.

What issues does this PR fix or reference?

Fixes: #51217

Previous Behavior

See referenced issue. Only plaintext and MD5 passwords supported.

New Behavior

If encrypted=scram-sha-256 is passed to the postgres_user.present or postgres_group.present states then the SCRAM-SHA-256 algorithm is used.

Other minor issues fixed:

  • If encryption is disabled by default, it cannot be enabled
  • Return says role was created when it was updated
  • Errors ignored from postgres_group.absent
  • Incorrect handling of group membership changes in postgres_group.present

Merge requirements satisfied?

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

@OrangeDog OrangeDog requested a review from a team as a code owner November 27, 2020 14:00
@OrangeDog OrangeDog requested review from garethgreenaway and removed request for a team November 27, 2020 14:00
@OrangeDog
Copy link
Collaborator Author

I did a manual integration test. If it's possible for someone to add something with some postgres Docker images that would be awesome.

@sagetherage sagetherage added Aluminium Release Post Mg and Pre Si Feature new functionality including changes to functionality and code refactors, etc. labels Dec 14, 2020
@sagetherage
Copy link
Contributor

Since this is a new feature and is pretty cool, can we get it added to Release Notes?

@OrangeDog
Copy link
Collaborator Author

Since this is a new feature and is pretty cool, can we get it added to Release Notes?

(How) do I do that?

@garethgreenaway
Copy link
Member

@OrangeDog You'll want to add some information to the release notes for 3003, which can be found here: salt/doc/topics/releases/3003.rst within the code base.

@garethgreenaway
Copy link
Member

@OrangeDog Apologies for the delay in getting to this. Since we missed the 3003 release, would you be able to update the PR to have the note in release notes for the Aluminum release? Thanks!

@OrangeDog
Copy link
Collaborator Author

But Aluminium is 3003...

@sagetherage
Copy link
Contributor

I think Gareth might be confused in that comment. @OrangeDog there was a test that cancelled out, but I have updated this PR with the master branch and that will re-run those and we can look again at getting this merged soon for Aluminium or 3003 release.

@OrangeDog Apologies for the delay in getting to this. Since we missed the 3003 release, would you be able to update the PR to have the note in release notes for the Aluminum release? Thanks!

@sagetherage
Copy link
Contributor

@Ch3LL this is looking close

@garethgreenaway
Copy link
Member

Looks good. @Ch3LL when you get a chance this one is ready to go.

salt/ext/saslprep.py Show resolved Hide resolved
tests/pytests/unit/modules/test_postgres.py Show resolved Hide resolved
import salt.states.postgres_group as postgres_group
from tests.support.mock import create_autospec, patch

DB_ARGS = {
Copy link
Contributor

Choose a reason for hiding this comment

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

same as here

@Ch3LL Ch3LL requested a review from dwoz February 25, 2021 18:48
salt/states/postgres_user.py Outdated Show resolved Hide resolved
tests/filename_map.yml Show resolved Hide resolved
tests/filename_map.yml Show resolved Hide resolved
OrangeDog and others added 2 commits February 26, 2021 21:04
Co-authored-by: Pedro Algarvio <pedro@algarvio.me>
salt/states/postgres_group.py Outdated Show resolved Hide resolved
Co-authored-by: Pedro Algarvio <pedro@algarvio.me>
@OrangeDog OrangeDog requested a review from s0undt3ch March 1, 2021 15:19
@OrangeDog
Copy link
Collaborator Author

It looks like adding the lines back to filename_map.yml has resulted in The command line is too long on Windows.
Nothing can be done about that here - the whole test running system will need a re-work unless there's an easy way to disable the limit.

@s0undt3ch
Copy link
Member

It looks like adding the lines back to filename_map.yml has resulted in The command line is too long on Windows.
Nothing can be done about that here - the whole test running system will need a re-work unless there's an easy way to disable the limit.

That is correct and a limitation on Windows which should be addressed.

Right now, I've triggered the full test suite run for both Windows which bypasses this problem

@dwoz
Copy link
Contributor

dwoz commented Mar 1, 2021

It looks like adding the lines back to filename_map.yml has resulted in The command line is too long on Windows.
Nothing can be done about that here - the whole test running system will need a re-work unless there's an easy way to disable the limit.

That is correct and a limitation on Windows which should be addressed.

Right now, I've triggered the full test suite run for both Windows which bypasses this problem

I'm going to merge this for Aluminum, @OrangeDog Please create a ticket for the issue which needs to be addressed on windows.

@dwoz dwoz merged commit 1e219c9 into saltstack:master Mar 1, 2021
@OrangeDog OrangeDog deleted the postgres_scram branch March 1, 2021 21:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Aluminium Release Post Mg and Pre Si Feature new functionality including changes to functionality and code refactors, etc. Tests-Passed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

SCRAM-SHA-256 support for states.postgres_user
7 participants