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

Rc vebt 777 revised - draft PR 2 #19592

Merged
merged 85 commits into from
Dec 3, 2024
Merged

Conversation

GcioGregg
Copy link
Contributor

Summary

  • *This work is behind a feature toggle (flipper): NO
  • Vye api dgib - Add API's to connect to four DGIB endpoints for VYE verifications
  • (If bug, how to reproduce) N/A
  • need API's for front end to call to get and post verification data to DGIB endpoints
  • VEBT
  • (If introducing a flipper, what is the success criteria being targeted?) N/A

Related issue(s)

Testing done

  • New code is covered by unit tests
  • did not have API's to DGIB endpoints
  • QA logs in with a test user, goes to /education/verify-school-enrollement/mgib-enrollments/, confirms that claimant id and verfiication data populates and verify claimant works as expected.
  • If this work is behind a flipper: N/A
    • Tests need to be written for both the flipper on and flipper off scenarios. Docs.
    • What is the testing plan for rolling out the feature? QA to test on staging, if works as expected, deploy to production. Note: front end related work that calls these API's is behind a feature flag.

What areas of the site does it impact?

VYE enrollment verifications

Acceptance criteria

  • I fixed|updated|added unit tests and integration tests for each feature (if applicable).
  • No error nor warning in the console.
  • Events are being sent to the appropriate logging solution
  • Documentation has been updated (link to documentation)
  • No sensitive information (i.e. PII/credentials/internal URLs/etc.) is captured in logging, hardcoded, or specs
  • Feature/bug has a monitor built into Datadog (if applicable)
  • If app impacted requires authentication, did you login to a local build and verify all authenticated routes work as expected

@va-vfs-bot va-vfs-bot temporarily deployed to rc-vebt-777-revised-draft-pr-2/main/main December 3, 2024 18:58 Inactive
@va-vfs-bot va-vfs-bot temporarily deployed to rc-vebt-777-revised-draft-pr-2/main/main December 3, 2024 19:15 Inactive
@RachalCassity
Copy link
Member

@GcioGregg Merging danger PR

@RachalCassity RachalCassity merged commit 4a809a1 into master Dec 3, 2024
29 of 30 checks passed
@RachalCassity RachalCassity deleted the rc-vebt-777-revised-draft-pr-2 branch December 3, 2024 20:15
@rjohnson2011
Copy link
Contributor

Need to revert this PR -> getting a failure on development/staging

DEPRECATION WARNING: Using preview_path= option is deprecated and will be removed in Rails 7.2. Please use preview_paths= instead. (called from <top (required)> at /app/config/environment.rb:7)
13
bin/rails aborted!
12
OpenSSL::PKey::RSAError: Neither PUB key nor PRIV key (OpenSSL::PKey::RSAError)
11
/app/lib/vye/dgib/authentication_token_service.rb:12:in `<class:AuthenticationTokenService>'
10
/app/lib/vye/dgib/authentication_token_service.rb:5:in `<module:DGIB>'
9
/app/lib/vye/dgib/authentication_token_service.rb:4:in `<module:Vye>'
8
/app/lib/vye/dgib/authentication_token_service.rb:3:in `<top (required)>'
7
/app/lib/vye/dgib/service.rb:6:in `require_relative'
6
/app/lib/vye/dgib/service.rb:6:in `<top (required)>'
5
/app/config/initializers/breakers.rb:9:in `<top (required)>'
4
/app/config/environment.rb:7:in `<top (required)>'
3
Tasks: TOP => db:migrate => db:load_config => environment
2
(See full trace by running task with --trace)
1
I, [2024-12-03T21:05:07.820556 #1]  INFO -- ddtrace: [ddtrace] DATADOG CONFIGURATION - CORE - {"date":"2024-12-03T21:05:07Z","os_name":"x86_64-pc-linux","version":"1.23.2","lang":"ruby","lang_version":"3.3.3","env":"eks-dev","service":"rails","dd_version":"ab23ab0e9ac1bb771c4b1de202c27c3e17ed3c7d","debug":false,"tags":"env:eks-dev,version:ab23ab0e9ac1bb771c4b1de202c27c3e17ed3c7d","runtime_metrics_enabled":false,"vm":"ruby-3.3.3","health_metrics_enabled":false,"profiling_enabled":false}

rjohnson2011 added a commit that referenced this pull request Dec 3, 2024
@bosawt
Copy link
Contributor

bosawt commented Dec 3, 2024

Need to revert this PR -> getting a failure on development/staging

DEPRECATION WARNING: Using preview_path= option is deprecated and will be removed in Rails 7.2. Please use preview_paths= instead. (called from <top (required)> at /app/config/environment.rb:7)
13
bin/rails aborted!
12
OpenSSL::PKey::RSAError: Neither PUB key nor PRIV key (OpenSSL::PKey::RSAError)
11
/app/lib/vye/dgib/authentication_token_service.rb:12:in `<class:AuthenticationTokenService>'
10
/app/lib/vye/dgib/authentication_token_service.rb:5:in `<module:DGIB>'
9
/app/lib/vye/dgib/authentication_token_service.rb:4:in `<module:Vye>'
8
/app/lib/vye/dgib/authentication_token_service.rb:3:in `<top (required)>'
7
/app/lib/vye/dgib/service.rb:6:in `require_relative'
6
/app/lib/vye/dgib/service.rb:6:in `<top (required)>'
5
/app/config/initializers/breakers.rb:9:in `<top (required)>'
4
/app/config/environment.rb:7:in `<top (required)>'
3
Tasks: TOP => db:migrate => db:load_config => environment
2
(See full trace by running task with --trace)
1
I, [2024-12-03T21:05:07.820556 #1]  INFO -- ddtrace: [ddtrace] DATADOG CONFIGURATION - CORE - {"date":"2024-12-03T21:05:07Z","os_name":"x86_64-pc-linux","version":"1.23.2","lang":"ruby","lang_version":"3.3.3","env":"eks-dev","service":"rails","dd_version":"ab23ab0e9ac1bb771c4b1de202c27c3e17ed3c7d","debug":false,"tags":"env:eks-dev,version:ab23ab0e9ac1bb771c4b1de202c27c3e17ed3c7d","runtime_metrics_enabled":false,"vm":"ruby-3.3.3","health_metrics_enabled":false,"profiling_enabled":false}

This means the certs weren't defined in application manifests. Is there a reason that wasn't done before this PR was merged?

@rmtolmach
Copy link
Contributor

rmtolmach commented Dec 5, 2024

Need to revert this PR -> getting a failure on development/staging

DEPRECATION WARNING: Using preview_path= option is deprecated and will be removed in Rails 7.2. Please use preview_paths= instead. (called from <top (required)> at /app/config/environment.rb:7)
13
bin/rails aborted!
12
OpenSSL::PKey::RSAError: Neither PUB key nor PRIV key (OpenSSL::PKey::RSAError)
11
/app/lib/vye/dgib/authentication_token_service.rb:12:in `<class:AuthenticationTokenService>'
10
/app/lib/vye/dgib/authentication_token_service.rb:5:in `<module:DGIB>'
9
/app/lib/vye/dgib/authentication_token_service.rb:4:in `<module:Vye>'
8
/app/lib/vye/dgib/authentication_token_service.rb:3:in `<top (required)>'
7
/app/lib/vye/dgib/service.rb:6:in `require_relative'
6
/app/lib/vye/dgib/service.rb:6:in `<top (required)>'
5
/app/config/initializers/breakers.rb:9:in `<top (required)>'
4
/app/config/environment.rb:7:in `<top (required)>'
3
Tasks: TOP => db:migrate => db:load_config => environment
2
(See full trace by running task with --trace)
1
I, [2024-12-03T21:05:07.820556 #1]  INFO -- ddtrace: [ddtrace] DATADOG CONFIGURATION - CORE - {"date":"2024-12-03T21:05:07Z","os_name":"x86_64-pc-linux","version":"1.23.2","lang":"ruby","lang_version":"3.3.3","env":"eks-dev","service":"rails","dd_version":"ab23ab0e9ac1bb771c4b1de202c27c3e17ed3c7d","debug":false,"tags":"env:eks-dev,version:ab23ab0e9ac1bb771c4b1de202c27c3e17ed3c7d","runtime_metrics_enabled":false,"vm":"ruby-3.3.3","health_metrics_enabled":false,"profiling_enabled":false}

This means the certs weren't defined in application manifests. Is there a reason that wasn't done before this PR was merged?

@bosawt They were added to dev https://github.com/department-of-veterans-affairs/vsp-infra-application-manifests/pull/3255 and staging https://github.com/department-of-veterans-affairs/vsp-infra-application-manifests/pull/3247. The dev PR looks good to me 🤔 but two things stand out to me in the staging PR:

  1. The certs are mounted to different places than the ones in dev. This might be totally fine, but I wanted to call it out.
  2. public_ica11_rca2_key_path is missing from staging. It's defined as an empty string in this vets-api PR but doesn't exist in the staging manifest. Could this have caused the problem? edit: maybe not... The error starts at authentication_token_service.rb:12 which uses Settings.dgi.vye.jwt.private_key_path which is NOT public_ica11_rca2_key_path

@GcioGregg @RileyRPM

@rmtolmach
Copy link
Contributor

I just looked at the value /dsva-vagov/vets-api/dev/dgib/jwt.crt in AWS and there is a cert there and there's a newline after the cert, so that all looks right.

@rmtolmach
Copy link
Contributor

I just looked at the value /dsva-vagov/vets-api/dev/dgib/jwt.crt in AWS and there is a cert there and there's a newline after the cert, so that all looks right.

🤔 but should it be a cert? In your PR you have modules/vye/spec/fixtures/dgi_private_test.pem as a key -----BEGIN PRIVATE KEY----- whereas in AWS, it's a -----BEGIN CERTIFICATE-----. So maybe there's an issue there.

@bosawt
Copy link
Contributor

bosawt commented Dec 5, 2024

I just looked at the value /dsva-vagov/vets-api/dev/dgib/jwt.crt in AWS and there is a cert there and there's a newline after the cert, so that all looks right.

🤔 but should it be a cert? In your PR you have modules/vye/spec/fixtures/dgi_private_test.pem as a key -----BEGIN PRIVATE KEY----- whereas in AWS, it's a -----BEGIN CERTIFICATE-----. So maybe there's an issue there.

Someone can test this on staging by logging into a rails console on staging just faking a JWT.encode with the key. But yeah, it won't work with a cert, the JWT needs to be encoded with a private key

@GcioGregg
Copy link
Contributor Author

I just looked at the value /dsva-vagov/vets-api/dev/dgib/jwt.crt in AWS and there is a cert there and there's a newline after the cert, so that all looks right.

🤔 but should it be a cert? In your PR you have modules/vye/spec/fixtures/dgi_private_test.pem as a key -----BEGIN PRIVATE KEY----- whereas in AWS, it's a -----BEGIN CERTIFICATE-----. So maybe there's an issue there.

This is the direction we received from Trevor on 12/3, which we followed: +
SIGNING_KEY = Settings.dgi.vye.jwt.private_key_path

  •  RSA_PRIVATE = OpenSSL::PKey::RSA.new(File.read(SIGNING_KEY)) if File.exist?(SIGNING_KEY)
    

you need to define a fixture in the Settings.vye.jwt.public_key_path and the private key path as well. I would recommend just copying what's in modules/meb_api/spec/fixtures/dgi_public_test.pem into a vye fixtures directory and pointing the settings to that

@GcioGregg
Copy link
Contributor Author

I just looked at the value /dsva-vagov/vets-api/dev/dgib/jwt.crt in AWS and there is a cert there and there's a newline after the cert, so that all looks right.

🤔 but should it be a cert? In your PR you have modules/vye/spec/fixtures/dgi_private_test.pem as a key -----BEGIN PRIVATE KEY----- whereas in AWS, it's a -----BEGIN CERTIFICATE-----. So maybe there's an issue there.

This is the direction we received from Trevor on 12/3, which we followed:

  • SIGNING_KEY = Settings.dgi.vye.jwt.private_key_path
  •  RSA_PRIVATE = OpenSSL::PKey::RSA.new(File.read(SIGNING_KEY)) if File.exist?(SIGNING_KEY)
    

you need to define a fixture in the Settings.vye.jwt.public_key_path and the private key path as well. I would recommend just copying what's in modules/meb_api/spec/fixtures/dgi_public_test.pem into a vye fixtures directory and pointing the settings to that

Let me know if that's not correct and what changes are needed. Also, do we need a value for the combined VA certs path: public_ica11_rca2_key_path: ""

@RileyRPM
Copy link

@bosawt is there a way we could analyze merging this today? I think Rachal may be OOO and the VYE team is looking to deploy. We think they've made all the requested changes. Can you help us get a pair of eyes on this and maybe meet with my guys? Thanks

@bosawt
Copy link
Contributor

bosawt commented Dec 12, 2024

@bosawt is there a way we could analyze merging this today? I think Rachal may be OOO and the VYE team is looking to deploy. We think they've made all the requested changes. Can you help us get a pair of eyes on this and maybe meet with my guys? Thanks

Hi Riley, I did a sanity check for the presence of the key in the proper format on the different stacks:

  • Dev -> RSA_PRIVATE = OpenSSL::PKey::RSA.new(File.read(Settings.dgi.vye.jwt.private_key_path))
    • This works, so the key appears to exist and be in valid format
  • Staging -> RSA_PRIVATE = OpenSSL::PKey::RSA.new(File.read(Settings.dgi.vye.jwt.private_key_path))
    • This works, so the key appears to exist and be in valid format
  • Prod -> key doesn't appear to exist:
image

I think once the prod key is resolved then the infrastructure necessary as a prerequisite to this PR will be in place. And then I don't see any issues to revisiting this PR after that

RachalCassity added a commit that referenced this pull request Dec 13, 2024
* Revert "Revert "Rc vebt 777 revised - draft PR 2 (#19592)" (#19692)"

This reverts commit a8981c0.

* add fake combined VA cert

---------

Co-authored-by: GcioGregg <117232882+GcioGregg@users.noreply.github.com>
@RileyRPM
Copy link

@bosawt , is there any way to push to staging without the prod key so that we can start testing with the VBA folks?

@GcioGregg
Copy link
Contributor Author

@bosawt , is there any way to push to staging without the prod key so that we can start testing with the VBA folks?

This PR has been deployed to staging. We will add the prod key later, once staging has been tested.

@RileyRPM
Copy link

@bosawt , is there any way to push to staging without the prod key so that we can start testing with the VBA folks?

This PR has been deployed to staging. We will add the prod key later, once staging has been tested.

TIGHT. Thanks

derekhouck pushed a commit that referenced this pull request Dec 31, 2024
* Revert "Revert "Rc vebt 777 revised - draft PR 2 (#19592)" (#19692)"

This reverts commit a8981c0.

* add fake combined VA cert

---------

Co-authored-by: GcioGregg <117232882+GcioGregg@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants