Skip to content

LG-4135: Upgrade Acuant SDK from v11.4.1 to v11.4.3#4628

Merged
mitchellhenke merged 1 commit intomasterfrom
aduth-lg-4135-acuant-11-4-3
Mar 3, 2021
Merged

LG-4135: Upgrade Acuant SDK from v11.4.1 to v11.4.3#4628
mitchellhenke merged 1 commit intomasterfrom
aduth-lg-4135-acuant-11-4-3

Conversation

@aduth
Copy link
Contributor

@aduth aduth commented Jan 28, 2021

Why: As a user, I expect that login.gov keeps their vendor dependencies up-to-date, so that bug fixes that may improve my likelihood of proofing success have been incorporated.

The change in acuant_sdk_initialization_endpoint is per migration notes. The SDK fails to initialize without this change. May want to double-check if the old CSP connect-src host is still relevant or needs to be updated. It's referenced in a couple other spots, but the SDK appears to load correctly for me without including the new hostname in CSP, even without development-specific allowances.

**Why**: As a user, I expect that login.gov keeps their vendor dependencies up-to-date, so that bug fixes that may improve my likelihood of proofing success have been incorporated.
@aduth
Copy link
Contributor Author

aduth commented Jan 28, 2021

Something which occurs to me: Even though we include the version as part of the path for the main SDK file (/acuant/11.4.3/AcuantJavascriptWebSdk.min.js) for cache-busting purposes, the SDK's internal loading of the worker script is a constant relative path (/verify/doc_auth/AcuantImageProcessingWorker.min.js), one which we also set a far-future expiration on (1 month in production).

As we learned already, we don't really have much control over how this script is loaded. But we could potentially override our response headers for this specific path to avoid setting cache headers? Doesn't help anyone who already has it cached though...

@zachmargolis
Copy link
Contributor

As we learned already, we don't really have much control over how this script is loaded. But we could potentially override our response headers for this specific path to avoid setting cache headers? Doesn't help anyone who already has it cached though...

Would it be worth seeing if we can set an E-Tag for that? So it's a content based cache and not just a time-based one?

Assuming that the assets are served by Rails in prod (I think they're served by NGINX now that they're in public)... we could just update our last mile middleware to do whatever we want to the cache expiration headers

@aduth
Copy link
Contributor Author

aduth commented Jan 29, 2021

Would it be worth seeing if we can set an E-Tag for that? So it's a content based cache and not just a time-based one?

Assuming that the assets are served by Rails in prod (I think they're served by NGINX now that they're in public)... we could just update our last mile middleware to do whatever we want to the cache expiration headers

I looked into this a bit. There's quite a bit going on, so it's hard to get my head around. Especially the differences between development and production.

Long story short: In production, we do set an ETag header for this file:

curl -I -s https://secure.login.gov/verify/doc_auth/AcuantImageProcessingWorker.min.js | grep etag:
etag: "601356f0-2ca07"

Per your comment, we'd still need to change the Cache-Control header to force it to validate, as currently it's set far in the future:

curl -I -s https://secure.login.gov/verify/doc_auth/AcuantImageProcessingWorker.min.js | grep cache-control:
cache-control: max-age=2592000
cache-control: public

The middleware you referenced would be a convenient spot to do this:

diff --git a/config/initializers/secure_headers.rb b/config/initializers/secure_headers.rb
index a3c89de68..3ab2acfec 100644
--- a/config/initializers/secure_headers.rb
+++ b/config/initializers/secure_headers.rb
@@ -87,6 +87,7 @@ class SecureHeaders::RemoveContentSecurityPolicy
 
     if (200...300).cover?(status) && @block.call(Rack::Request.new(env))
       headers.delete('Content-Security-Policy')
+      headers['Cache-Control'] = 'no-cache'
     end
 
     [status, headers, body]

Couple caveats:

  • It would need to take precedence over our NGINX Cache-Control header. I'm not sure we can be confident this will happen.
  • While it's a convenient reuse of modifying headers for the specific file, secure_headers.rb doesn't seem the most appropriate spot to define our cache behaviors.

If NGINX's header would override the one we set in code here, then we might just need to make an exception in the NGINX configuration for this file?

@zachmargolis
Copy link
Contributor

  • It would need to take precedence over our NGINX Cache-Control header. I'm not sure we can be confident this will happen.

NGINX is the outermost layer, my guess is these files in deployed environments are being served directly by NGINX and not even touching Rails, so ya we probably need to update those configs

@aduth
Copy link
Contributor Author

aduth commented Feb 1, 2021

I've opened a pull request in the DevOps repo proposing to change the configuration. We may want to consider this blocked 'til then, and possibly enough time for existing cache copies to expire in time for the upgraded Acuant to be live.

@aduth
Copy link
Contributor Author

aduth commented Feb 26, 2021

Update: This can be merged on or after Thursday, March 18, 2021 at 6:00 pm EDT

@mitchellhenke
Copy link
Contributor

Update: This can be merged on or after Thursday, March 18, 2021 at 6:00 pm EDT

Merging early to do some testing in live environments

@mitchellhenke mitchellhenke merged commit 3eac9d4 into master Mar 3, 2021
@mitchellhenke mitchellhenke deleted the aduth-lg-4135-acuant-11-4-3 branch March 3, 2021 15:45
mitchellhenke pushed a commit that referenced this pull request Mar 4, 2021
@mitchellhenke mitchellhenke restored the aduth-lg-4135-acuant-11-4-3 branch March 4, 2021 14:49
@aduth aduth deleted the aduth-lg-4135-acuant-11-4-3 branch March 4, 2021 15:05
mitchellhenke pushed a commit that referenced this pull request Mar 4, 2021
@mitchellhenke mitchellhenke restored the aduth-lg-4135-acuant-11-4-3 branch March 4, 2021 15:07
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.

3 participants