Skip to content

Remove plaintext password support for api#2588

Merged
ryan-pratt merged 19 commits intorelease/v7.0.0from
maint/remove-plaintext-pw
Dec 9, 2025
Merged

Remove plaintext password support for api#2588
ryan-pratt merged 19 commits intorelease/v7.0.0from
maint/remove-plaintext-pw

Conversation

@ryan-pratt
Copy link
Copy Markdown
Contributor

closes #2461

Checking with CURL using a token:

❯ curl -d '{"jsonrpc": "2.0", "method": "tlm", "params": ["INST HEALTH_STATUS TEMP1"], "id": 2, "keyword_params":{"type":"WITH_UNITS","scope":"DEFAULT"}}' \
-H "Content-Type: application/json" \
-H "Authorization: eAhY9MHvik5UitJ1hQ-r5g" \
http://localhost:2900/openc3-api/api | jq
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100   186  100    44  100   142   4229  13648 --:--:-- --:--:-- --:--:-- 18600
{
  "jsonrpc": "2.0",
  "id": 2,
  "result": "33.489 C"
}

and using the password:

❯ curl -d '{"jsonrpc": "2.0", "method": "tlm", "params": ["INST HEALTH_STATUS TEMP1"], "id": 2, "keyword_params":{"type":"WITH_UNITS","scope":"DEFAULT"}}' \
-H "Content-Type: application/json" \
-H "Authorization: password" \
http://localhost:2900/openc3-api/api | jq
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100  7538  100  7396  100   142   974k  19163 --:--:-- --:--:-- --:--:-- 1051k
{
  "jsonrpc": "2.0",
  "id": 2,
  "error": {
    "code": -32500,
    "message": "Password is invalid",
    "data": {
      "class": "OpenC3::AuthError",
      "message": "Token is invalid",
      "backtrace": [
        "/usr/lib/ruby/gems/3.3.0/gems/openc3-6.9.3.pre.beta0/lib/openc3/utilities/authorization.rb:46:in `authorize'",
        "/usr/lib/ruby/gems/3.3.0/gems/openc3-6.9.3.pre.beta0/lib/openc3/api/tlm_api.rb:82:in `tlm'",
        "/usr/lib/ruby/gems/3.3.0/gems/openc3-6.9.3.pre.beta0/lib/openc3/io/json_drb.rb:266:in `public_send'",
        "/usr/lib/ruby/gems/3.3.0/gems/openc3-6.9.3.pre.beta0/lib/openc3/io/json_drb.rb:266:in `process_request'",
        "/src/app/controllers/api_controller.rb:118:in `handle_post'",
        "/src/app/controllers/api_controller.rb:67:in `block in api'",
        "/usr/lib/ruby/gems/3.3.0/gems/openc3-6.9.3.pre.beta0/lib/openc3/utilities/open_telemetry.rb:52:in `in_span'",
        "/src/app/controllers/api_controller.rb:53:in `api'",
        "/usr/lib/ruby/gems/3.3.0/gems/actionpack-7.2.3/lib/action_controller/metal/basic_implicit_render.rb:8:in `send_action'",
        <truncated>
       ```

@codecov
Copy link
Copy Markdown

codecov bot commented Dec 2, 2025

Codecov Report

❌ Patch coverage is 57.14286% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 79.15%. Comparing base (3ff5fa0) to head (435b45d).
⚠️ Report is 132 commits behind head on release/v7.0.0.

Files with missing lines Patch % Lines
...mos-cmd-tlm-api/app/controllers/auth_controller.rb 57.14% 3 Missing ⚠️
Additional details and impacted files
@@                Coverage Diff                 @@
##           release/v7.0.0    #2588      +/-   ##
==================================================
- Coverage           79.70%   79.15%   -0.55%     
==================================================
  Files                 658      661       +3     
  Lines               50412    51740    +1328     
  Branches              736      735       -1     
==================================================
+ Hits                40179    40953     +774     
- Misses              10153    10707     +554     
  Partials               80       80              
Flag Coverage Δ
python 81.28% <ø> (-0.51%) ⬇️
ruby-api 84.36% <57.14%> (-0.14%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@ryan-pratt ryan-pratt marked this pull request as draft December 3, 2025 00:00
Copy link
Copy Markdown
Member

@jmthomas jmthomas left a comment

Choose a reason for hiding this comment

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

Seems like a lot of terminology in the code changing from token to password and vice versa (in some places). Are you good with all the name changes? Where is the actual change to force tokens instead of passwords?

@ryanmelt
Copy link
Copy Markdown
Member

ryanmelt commented Dec 3, 2025

Sha2 is insecure and we shouldn't be using it. Change to sha256

@ryan-pratt
Copy link
Copy Markdown
Contributor Author

Seems like a lot of terminology in the code changing from token to password and vice versa (in some places). Are you good with all the name changes? Where is the actual change to force tokens instead of passwords?

The main change is moving the block marked with # Check Direct password out of verify_no_service to its own function, verify_password. This new function is called by the login flow, and that should be pretty much it.

I feel pretty strongly about these renames. Before, token could have either been a token or a password, which made this code hard to follow. Tokens are passwords are handled differently now, so the variable names need to reflect that.

@ryan-pratt ryan-pratt force-pushed the maint/remove-plaintext-pw branch from 83a98cf to eecd594 Compare December 5, 2025 20:41
@socket-security
Copy link
Copy Markdown

socket-security bot commented Dec 5, 2025

Warning

Review the following alerts detected in dependencies.

According to your organization's Security Policy, it is recommended to resolve "Warn" alerts. Learn more about Socket for GitHub.

Action Severity Alert  (click "▶" to expand/collapse)
Warn High
Obfuscated code: npm svelte2tsx is 91.0% likely obfuscated

Confidence: 0.91

Location: Package overview

From: ?npm/@smui/common@7.0.0npm/@smui/list@7.0.0npm/@smui/menu@7.0.0npm/@smui/button@7.0.0npm/@smui/card@7.0.0npm/svelte2tsx@0.7.45

ℹ Read more on: This package | This alert | What is obfuscated code?

Next steps: Take a moment to review the security alert above. Review the linked package source code to understand the potential risk. Ensure the package is not malicious before proceeding. If you're unsure how to proceed, reach out to your security team or ask the Socket team for help at support@socket.dev.

Suggestion: Packages should not obfuscate their code. Consider not using packages with obfuscated code.

Mark the package as acceptable risk. To ignore this alert only in this pull request, reply with the comment @SocketSecurity ignore npm/svelte2tsx@0.7.45. You can also ignore all packages with @SocketSecurity ignore-all. To ignore an alert for all future pull requests, use Socket's Dashboard to change the triage state of this alert.

View full report

@ryan-pratt
Copy link
Copy Markdown
Contributor Author

I feel pretty strongly about these renames

Nevermind, I have managed to demoralize myself regarding this

@ryan-pratt
Copy link
Copy Markdown
Contributor Author

Ended up reverting everything and starting over with a much simpler approach. The original PR description still applies as-is wrt how to check this change.


def self.hash(token)
Digest::SHA2.hexdigest token
Digest::SHA256.hexdigest token
Copy link
Copy Markdown
Contributor Author

@ryan-pratt ryan-pratt Dec 5, 2025

Choose a reason for hiding this comment

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

SHA-256 is the default algorithm for the SHA2 family in Ruby's Digest module. We decided to use SHA-256, so this code change just makes that explicit. Users won't need to reset their password.

@ryan-pratt ryan-pratt marked this pull request as ready for review December 5, 2025 21:14
@ryan-pratt ryan-pratt requested a review from jmthomas December 5, 2025 21:14
@ryan-pratt ryan-pratt marked this pull request as draft December 5, 2025 21:34
@ryan-pratt
Copy link
Copy Markdown
Contributor Author

ryan-pratt commented Dec 5, 2025

back to draft, forgot to check the cli and it's broken

... also now I remember why I did it the first way the first time

@ryan-pratt ryan-pratt force-pushed the maint/remove-plaintext-pw branch from 415e032 to a1e39d6 Compare December 8, 2025 23:33
@ryan-pratt
Copy link
Copy Markdown
Contributor Author

Only remaining test failure is unrelated and happens on base release/v7.0.0 branch (openc3/spec/microservices/interface_microservice_spec.rb:139 # OpenC3::InterfaceMicroservice initialize creates an interface, updates status, and starts cmd thread)

@ryan-pratt ryan-pratt marked this pull request as ready for review December 9, 2025 00:54
Copy link
Copy Markdown
Member

@jmthomas jmthomas left a comment

Choose a reason for hiding this comment

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

There's a blurb in the cli.md that says: "Note that you must set the OPENC3_API_PASSWORD in COSMOS Core and both the OPENC3_API_USER and OPENC3_API_PASSWORD in COSMOS Enterprise" That still holds right?

Are there any ENV vars that are now "tokens" and not passwords or are they all still passwords?

@ryan-pratt
Copy link
Copy Markdown
Contributor Author

Are there any ENV vars that are now "tokens" and not passwords or are they all still passwords?

There are no no changes to the environment variables. The only time a user should have to think about tokens is if they're hitting the API directly (e.g. with curl), so the documentation there has been updated in this PR to tell them how to get a token and where to put it.

@ryan-pratt ryan-pratt merged commit 81ebb59 into release/v7.0.0 Dec 9, 2025
26 of 28 checks passed
@ryan-pratt ryan-pratt deleted the maint/remove-plaintext-pw branch December 9, 2025 19:07
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.

4 participants