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

feat: Typer CLI implementation with cognito device authorization flow #93

Open
wants to merge 25 commits into
base: main
Choose a base branch
from

Conversation

suhussai
Copy link
Contributor

@suhussai suhussai commented Sep 7, 2024

Reason for this change

Builders want more flexible methods of interacting with the SBT Control plane and a CLI client provides them an additional way to leverage the platform.

Description of changes

  • Implemented CLI and published as an PyPI package sbt-aws-cli.
  • Chose Typer framework due to its enhanced user interface and ease of development.
  • Set up a GitHub Actions workflow to automate the package build and publish process, ensuring that any updates to the CLI are automatically deployed to PyPI upon changes to the pyproject.toml file.
  • Added documentation in README for the CLI
  • Created cli-auth.ts which houses resources for OAuth 2.0 device authorization flow to ensure secure authentication for CLI users

Description of how you validated changes

  • Manually tested the CLI commands to ensure they behave as expected and interact correctly with the underlying services.
  • Validated the OAuth 2.0 device authorization flow to ensure it handles authentication securely and appropriately.
  • Ran the GitHub Actions workflow to confirm that the build and publish process works correctly
  • Added bash script to test the new CLI

Checklist

  • My code adheres to the CONTRIBUTING GUIDE
  • I have updated the relevant documentation (if applicable).

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of the project license.

Copy link
Contributor

@tobuck-aws tobuck-aws left a comment

Choose a reason for hiding this comment

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

While I think all of this is super cool, it does make me a little uneasy as we're effectively writing our own IdP, which is something I would tend to avoid. It would be good to get a security review of this before merging.

To access the help mode for the CLI, simply append the `--help` flag to any command. For example:

```bash
sbt-aws-cli --help
Copy link
Contributor

Choose a reason for hiding this comment

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

Does the actual executable have to be called sbt-aws-cli? Couldn't we make it just sbt-aws or even sbt?

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't know for certain, but I think a lot of the funcs in this file are utilities native to Javascript. btoa and atob are two examples. There might be others, including the url decoding/encoding. I would prefer to use a library for this stuff, rather than re-implement ourselves (especially with no tests). Thoughts?


//Function a random string based of the required lenght and format
// length: length of the random string to generate
// client_id: format of the randrom string to generate
Copy link
Contributor

Choose a reason for hiding this comment

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

I left an overall comment on this file, but this func specifically seems like it could pulled from uuid? Also, re: docs, there is no client_id parameter. Chars?

print("Configuring with:")
print(f"CONTROL_PLANE_STACK_NAME: {control_plane_stack}")
print(f"CLIENT_ID: {client_id}")
print(f"CLIENT_SECRET: {client_secret}")

Check failure

Code scanning / CodeQL

Clear-text logging of sensitive information High

This expression logs
sensitive data (secret)
as clear text.

Copilot Autofix AI about 1 month ago

To fix the problem, we need to ensure that sensitive information such as client_secret is not logged in clear text. Instead of logging the actual value, we can log a placeholder or a masked version of the sensitive data. This way, we maintain the ability to debug without exposing sensitive information.

The best way to fix this issue without changing existing functionality is to replace the logging of client_secret with a masked version. We can replace the actual value with a string indicating that it is a secret, such as "****" or "REDACTED".

Suggested changeset 1
scripts/sbt-aws-cli/sbt_aws_cli/main.py

Autofix patch

Autofix patch
Run the following command in your local git repository to apply this patch
cat << 'EOF' | git apply
diff --git a/scripts/sbt-aws-cli/sbt_aws_cli/main.py b/scripts/sbt-aws-cli/sbt_aws_cli/main.py
--- a/scripts/sbt-aws-cli/sbt_aws_cli/main.py
+++ b/scripts/sbt-aws-cli/sbt_aws_cli/main.py
@@ -116,3 +116,3 @@
         print(f"CLIENT_ID: {client_id}")
-        print(f"CLIENT_SECRET: {client_secret}")
+        print("CLIENT_SECRET: ****")
         print(f"FQDN: {fqdn}")
EOF
@@ -116,3 +116,3 @@
print(f"CLIENT_ID: {client_id}")
print(f"CLIENT_SECRET: {client_secret}")
print("CLIENT_SECRET: ****")
print(f"FQDN: {fqdn}")
Copilot is powered by AI and may make mistakes. Always verify output.
Positive Feedback
Negative Feedback

Provide additional feedback

Please help us improve GitHub Copilot by sharing more details about this comment.

Please select one or more of the options
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.

3 participants