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

First commits #1

Merged
merged 92 commits into from
Nov 12, 2021
Merged

First commits #1

merged 92 commits into from
Nov 12, 2021

Conversation

jsf9k
Copy link
Member

@jsf9k jsf9k commented Nov 4, 2021 •
edited
Loading

🗣 Description

This pull request contains the initial commits for this project.

💭 Motivation and context

This project's functionality is necessary for the RPT UAT taking place in the COOL.

See also:

🧪 Testing

I have applied these changes to env0 in our staging COOL environment and verified that they function as expected.

✅ Checklist

  • This PR has an informative and human-readable title.
  • Changes are limited to a single goal - eschew scope creep!
  • All future TODOs are captured in issues, which are referenced in code comments.
  • All relevant type-of-change labels have been added.
  • I have read the CONTRIBUTING document.
  • These code changes follow cisagov code standards.
  • All relevant repo and/or project documentation has been updated to reflect the changes in this PR.
  • Tests have been added and/or modified to cover the changes in this PR.
  • All new and existing tests pass.
  • Finalize version.

@jsf9k
Bare minimum functionality
9ab1e5c
@jsf9k
Allow VNC username, password, and private ssh key to be specified dir…
7734d7d 
…ectly
@jsf9k
Specify a region when using boto3
ad2a7fb
@jsf9k
Refine docopt/schema validation for VPC ID
7c0e1d8
@jsf9k
Add code to correctly add instances depending on OS type (Windows vs …
6403ae3 
…Linux)
@jsf9k
Add tests for stale (terminated) instances
6b9b5a2
@jsf9k
Remove two unnecessary lines from test code
736c92c
@jsf9k
Rename two test methods for clarity
e6fe8c8
@jsf9k
Add a couple of assert_not_called() assertions to test code
cdcdeb3
@jsf9k
Add a test for stopped instances
5258579
@jsf9k
Combine the two terminated instance tests
b2fd9fa 
They are really doing the same thing, and the OS doesn't matter in this case.
@jsf9k
Break out removal of connections by ID into its own function
8d13874 
Also add a new function for removing ghost instances, i.e. instances
in the DB that no longer exist in AWS.  Such a thing can happen if
instances are terminated while the Guacamole server is down.
@jsf9k
Remove call_count checks
4d9718f 
They do not appear to do what I thought they would.
@jsf9k
Bump version from 0.0.1 to 0.0.1-rc.1
ee5532a
@jsf9k
Fix location of version file
4b80cd0
@jsf9k
Improve function that checks for ghost instances
11bb595
@jsf9k
Improve handling of input arguments
1bd2d74
@jsf9k
Skip instances launched from AMIs that match any of a list of regexes
c9f73c2 
Also pre-compile all regexes where possible.
@jsf9k
Bump version from 0.0.1-rc.1 to 0.0.1-rc.2
1cefa6b
@jsf9k
Fix a bug in the way the IF of the default VPC is retrieved via insta…
bc01b3e 
…nce metadata
@jsf9k
Bump version from 0.0.1-rc.2 to 0.0.1-rc.3
2cfdb8b
@jsf9k
Add a --oneshot option that runs the update loop only once
1000145 
This is useful for testing, although it isn't the way the utility is usually run.
@jsf9k
Bump version from 0.0.1-rc.3 to 0.0.1-rc.4
3ba0f33
@jsf9k
Add an input parameter to specify the sleep between loop iterations
5c43c21
@jsf9k
Precompile the VPC ID regex
368a0fb
@jsf9k
Bump version from 0.0.1-rc.4 to 0.0.1-rc.5
c9c3396
@jsf9k
Move DB connection attempt inside the loop
21ac2ed 
This allows us to wrap it in a try block, so we can repeat the loop if
the connection fails.
@jsf9k
Bump version from 0.0.1-rc.5 to 0.0.1-rc.6
077359f
@jsf9k
Modify the connection name to be something more meaningful
20f5e39
@jsf9k
Bump version from 0.0.1-rc.6 to 0.0.1-rc.7
0229d0f
@jsf9k jsf9k requested review from mcdonnnj and dav3r November 10, 2021 18:35
@jsf9k
Remove debug logging statement that could contain sensitive information
6b2db01
@jsf9k
Avoid importing from psycopg in two different ways
d1eb68f 
LGTM complains about this on the grounds that it is confusing.
@jsf9k
Add an LGTM comment string to disable a warning
0842757 
We must use the same password hashing algorithm as is used in the
Guacamole source code, so we cannot avoid the LGTM warning.
@jsf9k
Do not include username in logging statement
da2df6b 
Our linters think this is logging of sensitive information, which is
reasonable.
@lgtm-com
Copy link

lgtm-com bot commented Nov 10, 2021

This pull request introduces 1 alert and fixes 1 when merging da2df6b into 361a06a - view on LGTM.com

new alerts:

  • 1 for Use of a broken or weak cryptographic hashing algorithm on sensitive data

fixed alerts:

  • 1 for Clear-text logging of sensitive information

@lgtm-com
Copy link

lgtm-com bot commented Nov 10, 2021

This pull request introduces 1 alert and fixes 1 when merging 70e3616 into 361a06a - view on LGTM.com

new alerts:

  • 1 for Use of a broken or weak cryptographic hashing algorithm on sensitive data

fixed alerts:

  • 1 for Clear-text logging of sensitive information

dav3r
dav3r approved these changes Nov 10, 2021
View reviewed changes
Copy link
Member

@dav3r dav3r left a comment

Choose a reason for hiding this comment

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

👍 👍

src/guacscanner/guacscanner.py Outdated Show resolved Hide resolved
src/guacscanner/guacscanner.py Outdated Show resolved Hide resolved
tests/test_guacscanner.py Outdated Show resolved Hide resolved
jsf9k and others added 2 commits November 10, 2021 15:09
@jsf9k @dav3r
Rename a variable to preserve alphabetical order and yet keep related…
6dd384b 
… variables together

Co-authored-by: dav3r <david.redmin@trio.dhs.gov>
@jsf9k @dav3r
Fix a typo
36025de 
Co-authored-by: dav3r <david.redmin@trio.dhs.gov>
@lgtm-com
Copy link

lgtm-com bot commented Nov 10, 2021

This pull request introduces 1 alert and fixes 1 when merging 36025de into 361a06a - view on LGTM.com

new alerts:

  • 1 for Use of a broken or weak cryptographic hashing algorithm on sensitive data

fixed alerts:

  • 1 for Clear-text logging of sensitive information

@jsf9k @mcdonnnj
Use a dataclass for passing Guacamole connection parameters as functi…
95de413 
…on inputs

This simplifies function signatures and makes modifying the list of
connection parameters less error-prone.

Co-authored-by: Nick M <50747025+mcdonnnj@users.noreply.github.com>
@jsf9k jsf9k requested review from dav3r and felddy November 11, 2021 04:16
@lgtm-com
Copy link

lgtm-com bot commented Nov 11, 2021

This pull request introduces 1 alert and fixes 1 when merging 95de413 into 361a06a - view on LGTM.com

new alerts:

  • 1 for Use of a broken or weak cryptographic hashing algorithm on sensitive data

fixed alerts:

  • 1 for Clear-text logging of sensitive information

mcdonnnj
mcdonnnj approved these changes Nov 12, 2021
View reviewed changes
Copy link
Member

@mcdonnnj mcdonnnj left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Strong work again 💪💪💪Thank you for taking my suggestions into consideration. I had one question below that isn't an immediate blocker.

src/guacscanner/guacscanner.py Outdated Show resolved Hide resolved
@jsf9k
Do not create DB connection in a context manager
270ac59 
@mcdonnnj pointed out that psycopg.connect() does not entirely act as
expected in that it does not automatically close the connection when
you exit the context.  It only rolls back transactions in the event
that one fails.

Since we are managing transactions in this code via explicit calls to
psycopg.Connection.commit(), the context manager isn't actually buying
us anything; therefore, I have gotten rid of it.

I added an explicit psycopg.Connection.close() at the bottom of the
loop.  I don't want to create the connection outside the loop and
leave it open because the guacscanner may be up and running for months
at a time, and who knows what could go wrong.  It seems safer to just
recreate the connection at the top of the loop.
@jsf9k
Bump version from 0.0.2-rc.1 to 0.0.2-rc.2
51bf9d2
@lgtm-com
Copy link

lgtm-com bot commented Nov 12, 2021

This pull request introduces 1 alert and fixes 1 when merging 51bf9d2 into 361a06a - view on LGTM.com

new alerts:

  • 1 for Use of a broken or weak cryptographic hashing algorithm on sensitive data

fixed alerts:

  • 1 for Clear-text logging of sensitive information

@lgtm-com
Copy link

lgtm-com bot commented Nov 12, 2021

This pull request introduces 1 alert and fixes 1 when merging 73e625f into 361a06a - view on LGTM.com

new alerts:

  • 1 for Use of a broken or weak cryptographic hashing algorithm on sensitive data

fixed alerts:

  • 1 for Clear-text logging of sensitive information

@lgtm-com
Copy link

lgtm-com bot commented Nov 12, 2021

This pull request introduces 1 alert and fixes 1 when merging d3887b2 into 361a06a - view on LGTM.com

new alerts:

  • 1 for Use of a broken or weak cryptographic hashing algorithm on sensitive data

fixed alerts:

  • 1 for Clear-text logging of sensitive information

@jsf9k jsf9k merged commit cbe72d6 into develop Nov 12, 2021
@jsf9k jsf9k deleted the first-commits branch November 12, 2021 21:30
@dav3r dav3r mentioned this pull request Jun 9, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mcdonnnj mcdonnnj mcdonnnj approved these changes

@dav3r dav3r Awaiting requested review from dav3r dav3r is a code owner

@felddy felddy Awaiting requested review from felddy felddy is a code owner

Assignees

@jsf9k jsf9k

Labels
improvement This issue or pull request will add or improve functionality, maintainability, or ease of use
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@jsf9k @coveralls @mcdonnnj @felddy @dav3r