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.

Sign up for GitHub

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

Jump to bottom

[security] Fix missing no_log=True #475

Merged
merged 1 commit into from
Mar 13, 2021
Merged

[security] Fix missing no_log=True #475

merged 1 commit into from
Mar 13, 2021

Conversation

felixfontein
Copy link
Contributor

@felixfontein felixfontein commented Mar 13, 2021 •
edited by tremble
Loading

SUMMARY

Fallout from the new no_log sanity tests. I've checked with @tremble and these seem to be really missing.

  • aws_direct_connect_virtual_interface's authentication_key parameter is arguably secret (BGP Authentication Key)
  • sts_assume_role's and sts_session_token's mfa_token is a one-time token; attackers could intercept the log call and use the token before it is used by the module.

CC @jillr @relrod

ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME

aws_direct_connect_virtual_interface
sts_assume_role
sts_session_token

@ansibullbot
Copy link

cc @bekelchik @piontas @pwnall @s-hertel @wimnat
click here for bot help

@ansibullbot ansibullbot added bug This issue/PR relates to a bug module module needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR needs_triage plugins plugin (any type) labels Mar 13, 2021
tremble
tremble approved these changes Mar 13, 2021
View reviewed changes
Copy link
Contributor

@tremble tremble left a comment

Choose a reason for hiding this comment

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

LGTM

@tremble tremble mentioned this pull request Mar 13, 2021
Merged
@tremble
Copy link
Contributor

tremble commented Mar 13, 2021

recheck

@felixfontein
Copy link
Contributor Author

felixfontein commented Mar 13, 2021 •
edited
Loading

@tremble that will still fail. ansible-collections/amazon.aws#303 needs to be merged first.

markuman
markuman approved these changes Mar 13, 2021
View reviewed changes
Copy link
Member

@markuman markuman left a comment

Choose a reason for hiding this comment

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

Once the mfa token is used, it cannot be used a 2nd time.
Imho leaking mfa token is not dramatically.

LGTM

@felixfontein
Copy link
Contributor Author

@markuman except if someone manages to use the token after it is logged to syslog (every module on startup logs its invocation including all parameters that don't have no_log=True to syslog) before the module itself manages to use it. It's a race condition, but a skilled attacked might be able to exploit this.

@relrod
Copy link

relrod commented Mar 13, 2021

Also consider a case where we log/show the parameter, but then for some reason the module crashes out. Now the MFA was logged/shown but never used. It needs to be no_log'd.

@tremble
Copy link
Contributor

tremble commented Mar 13, 2021

Agree that it's a risk and should not be logged. Difficult to exploit though.

@felixfontein
Copy link
Contributor Author

recheck

felixfontein added a commit to felixfontein/ansible that referenced this pull request Mar 13, 2021
@felixfontein
Backport of ansible-collections/community.aws#475.
f6688b3
@felixfontein felixfontein mentioned this pull request Mar 13, 2021
Merged
@felixfontein
Copy link
Contributor Author

I already created a backport PR for stable-2.9: ansible/ansible#73894

@ansibullbot ansibullbot added community_review and removed needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR labels Mar 13, 2021
@felixfontein felixfontein merged commit 26ad349 into ansible-collections:main Mar 13, 2021
@felixfontein felixfontein deleted the real-secrets branch March 13, 2021 21:00
@felixfontein
Copy link
Contributor Author

@tremble @markuman @relrod thanks everyone!

felixfontein added a commit to felixfontein/ansible that referenced this pull request Mar 15, 2021
@felixfontein
Backport of ansible-collections/community.aws#475.
7db5e4a
@felixfontein felixfontein mentioned this pull request Mar 15, 2021
Merged
relrod pushed a commit to ansible/ansible that referenced this pull request Apr 3, 2021
@felixfontein
Backport of ansible-collections/community.aws#475. (#73894)
7b7c219
relrod pushed a commit to ansible/ansible that referenced this pull request Apr 3, 2021
@felixfontein
Backport of ansible-collections/community.aws#475. (#73910)
6b8c35c
clrpackages pushed a commit to clearlinux-pkgs/ansible that referenced this pull request Apr 15, 2021
@phmccarty @clrpackages
ansible: Autospec creation for update from version 2.9.19 to version …
69e3890 
…2.9.20

Alina Buzachis (1):
      New AWS module mod_defaults - rds_option_group (_info) modules (#74098)

Carlos Camacho (1):
      [stable-2.9] Fix: nmcli bridge-slave fails with error (#74125)

Felix Fontein (4):
      Backport of ansible-collections/community.docker#103. (#73890)
      Backport of ansible-collections/community.aws#475. (#73894)
      Backport of ansible-collections/community.general#2018. (#73893)
      Backport of ansible-collections/community.network#223. (#73909)

Jill R (1):
      New AWS module mod_defaults - wafv2 modules (#73975)

Mark Chappell (3):
      Ensure unit test paths for connection and inventory plugins are based on the context (#73877)
      Partial backport of community.aws/471 - no_log=True for aws_secret (#73874)
      [backport/2.9] module_defaults: Add rds_snapshot (#74113)

Matt Clay (1):
      [stable-2.9] Fix ansible-test coverage exporting.

Matt Martz (1):
      [stable-2.9] Ensure task from the worker is finalized/squashed (#73881) (#73929)

Rick Elrod (5):
      Update Ansible release version to v2.9.19.post0.
      [security] Add more missing no_logs (#74115)
      New release v2.9.20rc1
      Update Ansible release version to v2.9.20rc1.post0.
      New release v2.9.20

Sam Doran (2):
      Move file needed by cs_volume test to S3
      [stable-2.9] find - set proper default based on use_regex (#73961) (#73966)

Xabier Napal (1):
      Fix wrong backup directory var name in apt module (#73840) (#74003)

nitzmahone (1):
      add optional module_utils import support (#73832) (#73916)
alinabuzachis pushed a commit to alinabuzachis/community.aws that referenced this pull request Jul 16, 2021
@felixfontein
[promoted]Merge pull request ansible-collections#475 from felixfontei…
126b074 
…n/real-secrets

[security] Fix missing no_log=True

This commit was initially merged in https://github.com/ansible-collections/community.aws
See: ansible-collections@26ad349
alinabuzachis pushed a commit to alinabuzachis/community.aws that referenced this pull request Jul 19, 2021
@felixfontein
Merge pull request ansible-collections#475 from felixfontein/real-sec…
b05e87a 
…rets

[security] Fix missing no_log=True
alinabuzachis pushed a commit to alinabuzachis/community.aws that referenced this pull request Jul 19, 2021
@felixfontein
Merge pull request ansible-collections#475 from felixfontein/real-sec…
3975641 
…rets

[security] Fix missing no_log=True
abikouo pushed a commit to abikouo/community.aws that referenced this pull request Oct 24, 2023
@tremble
Re-enable ec2_vpc_endpoint tests (ansible-collections#1167)
6446e6f 
Re-enable ec2_vpc_endpoint tests

SUMMARY
Tests were a little broken, fixed.
Also tried splitting out the VPC cleanup code to reduce copy&paste.
fixes: ansible-collections#475
ISSUE TYPE

Bugfix Pull Request

COMPONENT NAME
ec2_vpc_endpoint
ADDITIONAL INFORMATION

Reviewed-by: Mark Chappell <None>
Reviewed-by: Alina Buzachis <None>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@markuman markuman markuman approved these changes

@tremble tremble tremble approved these changes

Assignees
No one assigned
Labels
bug This issue/PR relates to a bug community_review module module plugins plugin (any type)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@felixfontein @ansibullbot @tremble @relrod @markuman