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

fix(SSL): separate each certificate into an individual item #2542

Merged

Conversation

BenjaminBruenau
Copy link
Contributor

@BenjaminBruenau BenjaminBruenau commented Mar 29, 2024

This fixes the issue described in #2541.

There seems to have been a mistake while updating the cert-chain in #2131 , which was released in 3.9.3.
The cert-chain was changed into a single string where before each of the certificates were separate array elements.

Copy link

codecov bot commented Mar 29, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 91.20%. Comparing base (a9c6c3e) to head (739d48f).

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #2542   +/-   ##
=======================================
  Coverage   91.20%   91.20%           
=======================================
  Files          69       69           
  Lines       15547    15547           
  Branches     1331     1330    -1     
=======================================
  Hits        14179    14179           
  Misses       1368     1368           
Flag Coverage Δ
compression-0 91.20% <100.00%> (ø)
compression-1 91.20% <100.00%> (ø)
tls-0 90.71% <100.00%> (ø)
tls-1 91.02% <100.00%> (ø)

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.

@wellwelwel
Copy link
Collaborator

Thanks, @BenjaminBruenau!

I can't review it at this time, but based on recent reports, it looks good to me.

@wellwelwel wellwelwel changed the title Separated each certificate into a single array element fix(SSL): separate each certificate into an individual item Mar 30, 2024
@wellwelwel wellwelwel added the high-priority When in doubt what next pick this label label Mar 30, 2024
@BMO-tech
Copy link

Thanks for the update, I can test in on Monday when I'm back at work

@sidorares
Copy link
Owner

If someone from AWS reads this - would be great to have small RDS instance so that we can have SSL smoke test in the CI ( if you not from AWS but know who to tag - please do so 😝 )

@sidorares
Copy link
Owner

And thanks a lot for the fix @BenjaminBruenau . I think we can wait for extra confirmation from @BMO-tech before merging

@ennioVisco
Copy link

ennioVisco commented Mar 31, 2024

And thanks a lot for the fix @BenjaminBruenau . I think we can wait for extra confirmation from @BMO-tech before merging

As mentioned here, #2131 (comment), in our case it seems to brake the node modules import, still investigating how it is related. If we have updates, we'll report here.

@wellwelwel
Copy link
Collaborator

wellwelwel commented Apr 2, 2024

It's a really basic review: since there are 115 certs, this PR ensures 115 array items.

I'll merge it, since currently this is a single string (one array item) for 115 certificates (potentially causing tokenization limit issues - vercel/nft#402), but we'd appreciate to have an extra check (in a real RDS).

Thanks again, @BenjaminBruenau 🙋🏻‍♂️

cc: @BMO-tech, @pedrovanzella

@wellwelwel wellwelwel merged commit 63f1055 into sidorares:master Apr 2, 2024
64 checks passed
@wellwelwel wellwelwel removed the high-priority When in doubt what next pick this label label Apr 2, 2024
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.

None yet

5 participants