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 support for setting ACL rules on certificates #548

Merged
merged 22 commits into from
Sep 12, 2023

Conversation

jpitlor
Copy link
Contributor

@jpitlor jpitlor commented Aug 25, 2023

SUMMARY

I'm making this PR that essentially revives #94 to add support to Cert:\ paths in win_acl. I tried to follow all the unresolved feedback from #94

Fixes #150

ISSUE TYPE
  • Feature Pull Request
COMPONENT NAME

win_acl

ADDITIONAL INFORMATION

There is one huge caveat that I'm fairly certain my test assertions are wrong, but when I try to use ansible-test, I'm getting a very unhelpful error (I'll add more details in a comment), so I'm assuming that's going to block this getting merged. I realize this isn't an Ansible help forum, but if you happen to have any suggestions on what part of this stack trace is useful, I'd love to try and get these tests running

WARNING: Excluding target tests marked "unstable" which require --allow-unstable or prefixing with "unstable/": win_wait_for
Traceback (most recent call last):
  File "/home/jpitlor/.local/bin/ansible-test", line 45, in <module>
    main()
  File "/home/jpitlor/.local/bin/ansible-test", line 36, in main
    cli_main(args)
  File "/home/jpitlor/.local/lib/python3.10/site-packages/ansible_test/_internal/__init__.py", line 84, in main
    args.func(config)
  File "/home/jpitlor/.local/lib/python3.10/site-packages/ansible_test/_internal/commands/integration/windows.py", line 76, in command_windows_integration
    host_state, internal_targets = command_integration_filter(args, all_targets)
  File "/home/jpitlor/.local/lib/python3.10/site-packages/ansible_test/_internal/commands/integration/__init__.py", line 941, in command_integration_filter
    cloud_init(args, internal_targets)
  File "/home/jpitlor/.local/lib/python3.10/site-packages/ansible_test/_internal/commands/integration/cloud/__init__.py", line 162, in cloud_init
    provider.setup()
  File "/home/jpitlor/.local/lib/python3.10/site-packages/ansible_test/_internal/commands/integration/cloud/httptester.py", line 58, in setup
    descriptor = run_support_container(
  File "/home/jpitlor/.local/lib/python3.10/site-packages/ansible_test/_internal/containers.py", line 142, in run_support_container
    current_container_id = get_docker_container_id()
  File "/home/jpitlor/.local/lib/python3.10/site-packages/ansible_test/_internal/thread.py", line 59, in wrapper
    return func(*args, **kwargs)
  File "/home/jpitlor/.local/lib/python3.10/site-packages/ansible_test/_internal/util.py", line 158, in cache_func
    value = storage[None] = func()
  File "/home/jpitlor/.local/lib/python3.10/site-packages/ansible_test/_internal/docker_util.py", line 592, in get_docker_container_id
    mounts = MountEntry.loads(mountinfo_path.read_text())
  File "/home/jpitlor/.local/lib/python3.10/site-packages/ansible_test/_internal/cgroup.py", line 107, in loads
    return tuple(cls.parse(line) for line in value.splitlines())
  File "/home/jpitlor/.local/lib/python3.10/site-packages/ansible_test/_internal/cgroup.py", line 107, in <genexpr>
    return tuple(cls.parse(line) for line in value.splitlines())
  File "/home/jpitlor/.local/lib/python3.10/site-packages/ansible_test/_internal/cgroup.py", line 86, in parse
    assert separator == '-'
AssertionError

Jordan Pitlor added 4 commits August 25, 2023 09:09
This commit is copying the contents of an older abandoned merge request
by user Hakon, with some minor adjustments like removing dead code and
implementing some feedback from the original MR.

#94
In hindsight, as I am basically copying Hakon's code, it seems right
that he should get credit for it
The assertions are almost certainly wrong, but I wanted to commit anyway
to not accidentally revert the stuff that does work.
@jpitlor
Copy link
Contributor Author

jpitlor commented Aug 25, 2023

I do see I have some linting errors, but before I fix those, I just want to understand - if the Windows Server tasks are reporting successful, does that mean the integration tests are all actually passing?

@jborean93
Copy link
Collaborator

I do see I have some linting errors, but before I fix those, I just want to understand - if the Windows Server tasks are reporting successful, does that mean the integration tests are all actually passing?

That is correct, the tests are in group 1 and if they are green then the tests have passed :)

@jpitlor
Copy link
Contributor Author

jpitlor commented Aug 28, 2023

great! funny enough I see what happened now - I'm assuming because the lint failed, the integration tests didn't run, which would make sense.

I'll see what I can do to fix the integration tests. Hopefully I can get them runnning locally; I'd like to not abuse whatever Azure plan this is running on (even if it's free - I don't know if Azure gives free credits to FOSS)

@jborean93
Copy link
Collaborator

I'll see what I can do to fix the integration tests. Hopefully I can get them runnning locally; I'd like to not abuse whatever Azure plan this is running on (even if it's free - I don't know if Azure gives free credits to FOSS)

Simply create a file at tests/integration/inventory.winrm with a host called windows and then run ansible-test windows-integration win_acl and it will run those tests on the host in question. You can add -v, -vv, etc if you need higher verbosity.

@jpitlor
Copy link
Contributor Author

jpitlor commented Aug 28, 2023

Ahhhh, I figured out I needed that inventory file, but I hadn't realized the host needed to be called windows. I think I can figure out the rest. Thanks so much!

@jborean93
Copy link
Collaborator

Actually sorry the host can be called anything, it just needs to be in a group called windows.

@jpitlor
Copy link
Contributor Author

jpitlor commented Aug 30, 2023

holy moly, that was quite the experience. @jborean93 this should be ready for an actual review whenever you have time

On the bright side, the tests actually did catch a bug! There was no idempotency check in the initial PR, so I added that!

Copy link
Collaborator

@jborean93 jborean93 left a comment

Choose a reason for hiding this comment

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

Thanks for working on this and getting the tests running. I've added a few comments here. The overall implementation looks great, just had a few minor concerns/suggestions.

plugins/module_utils/CertACLHelper.cs Outdated Show resolved Hide resolved
plugins/module_utils/CertACLHelper.cs Outdated Show resolved Hide resolved
plugins/module_utils/CertACLHelper.cs Outdated Show resolved Hide resolved
plugins/module_utils/CertACLHelper.cs Outdated Show resolved Hide resolved
plugins/module_utils/CertACLHelper.cs Outdated Show resolved Hide resolved
plugins/module_utils/CertACLHelper.cs Outdated Show resolved Hide resolved
plugins/module_utils/CertACLHelper.cs Outdated Show resolved Hide resolved
plugins/module_utils/CertACLHelper.cs Outdated Show resolved Hide resolved
plugins/module_utils/CertACLHelper.cs Outdated Show resolved Hide resolved
plugins/module_utils/CertACLHelper.cs Outdated Show resolved Hide resolved
@jpitlor
Copy link
Contributor Author

jpitlor commented Sep 8, 2023

@jborean93 almost everything has been addressed - there is a question mark surrounding CRYPT_ACQUIRE_ALLOW_NCRYPT_KEY_FLAG vs CRYPT_ACQUIRE_PREFER_NCRYPT_KEY_FLAG. If you happen to know anything more about that (or have some innate ability to navigate the Microsoft docs website), I'd appreciate any insight, but if not, I'm planning on looking into it more next week.

@jborean93
Copy link
Collaborator

Thank you very much for working on this and working on the tests. It great to see a very in depth implementation with tests to cover them!

@jborean93 jborean93 merged commit 5f8f04b into ansible-collections:main Sep 12, 2023
24 checks passed
@opoplawski
Copy link

Thank you very much for this contribution! I've been manually patching our local install for a long time.

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.

RFE: Modify permissions of private keys in the certificate store
3 participants