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

Update Amazon Root 2019 RDS certificate #2280

Merged
merged 1 commit into from
Jan 21, 2020

Conversation

codebykenny
Copy link
Contributor

Amazon RDS has released their 2019-2024 CA’s and has given a formal warning to all RDS users to upgrade their CA’s to the new one before February 5th 2020.

All new RDS Instances will have the new 2019-2024 CA’s starting in November 2019.

You can read about it here:
https://docs.aws.amazon.com/AmazonRDS/latest/UserGuide/UsingWithRDS.SSL-certificate-rotation.html

And their CA’s are posted here:
https://docs.aws.amazon.com/AmazonRDS/latest/UserGuide/UsingWithRDS.SSL.html

This PR adds their new 2019-2024 Global CA that works for all AWS Regions

@dougwilson
Copy link
Member

In addition this just the profile; you can already supply this new cert directly in the "ca" config of this module, so the merging of this PR does not in any way prevent you from using this cert with any existing version of this module.

@AashishAsh
Copy link

@dougwilson We updated the cert on the rds cluster from ca-2015 to ca-2019 which went through. but the nodejs application which has a dependency on this ssl_profile is failed due to this cert is not merged yet.

Error:
unable to get local issuer certificate
Error: unable to get local issuer certificate

@dougwilson
Copy link
Member

dougwilson commented Oct 15, 2019

Thanks both of you :) so it sounds like there are some options

  1. Set your instance to use the older cert for now
  2. Just supply the new root cert in the ca option https://github.com/mysqljs/mysql/blob/master/Readme.md#ssl-options
  3. Wait for the new release

@mysqljs mysqljs deleted a comment from AashishAsh Jan 6, 2020
@mysqljs mysqljs deleted a comment from arkintz Jan 6, 2020
@mysqljs mysqljs deleted a comment from AashishAsh Jan 6, 2020
@mysqljs mysqljs deleted a comment from AashishAsh Jan 6, 2020
@mysqljs mysqljs deleted a comment from winzig Jan 6, 2020
@felixge
Copy link
Collaborator

felixge commented Jan 14, 2020

@dougwilson hey, I just saw a few people complain about this on [email protected]. I'm happy and grateful to let you run this project as you see fit these days, but I'm wondering what's going on here?

I understand that users can deal with this issue in their applications, but is there anything wrong with just merging this PR? And what's up with all the deleted comments and restricting the issue to repo contributors?

Anyway, you don't owe anybody any timely action on this, but I just wanted to check if everything is okay : ). If hitting merge & cutting a release is all that's needed to make people happy here, I'd also be willing to help with it.

@dougwilson
Copy link
Member

dougwilson commented Jan 14, 2020

I'm happy and grateful to let you run this project as you see fit these days, but I'm wondering what's going on here?

I am working to roll out a security update and roll this change into a second module to shared with mysql2.

And what's up with all the deleted comments and restricting the issue to repo contributors?

I was simply deleting all the "me too" and "+1" comments and out of date information I had posted to keep the thread clean and so it wasn't confusing to folks coming by. In the future I will use the "hide comment" feature instead, though it does make the thread still a but longer, but it is a newer feature that I should probably use for everyone's sanity.

Anyway, you don't owe anybody any timely action on this, but I just wanted to check if everything is okay : ). If hitting merge & cutting a release is all that's needed to make people happy here, I'd also be willing to help with it.

Splitting out the profile into a separate module will be better in the long run which is why I'm doing that. It will mean mysql and mysql2 will be able to not have duplicate PRs and we can update the profile more streamline.

As a post script, I am heading to work now, but if this is a hot topic I will merge and release this change at work later today and delay the security fixes until a later release.

P.P.S (sorry, I'm trying not to clutter up this issue, but figure folks are looking here currently) there are is a backlog of things that generally need a 3.0 release, and prior to this I was working to get it put together. I will be opening a meta issue to track and though 2.0 was a good long run, it is time to close out a lot of outstanding issues/features in a 3.0.

@felixge
Copy link
Collaborator

felixge commented Jan 14, 2020

@dougwilson sounds good, I'll let you handle it : ).

That being said, I'm kinda disappointed. I was hoping for some nice conspiracy. This could have been the beginning of the uprising against Jeff Bezos! TBH I'd probably support that ; ).

Anyway, thanks for all the work on this project! The node community is lucky to have you.

@dougwilson dougwilson self-assigned this Jan 14, 2020
@bradennapier

This comment has been minimized.

@dougwilson

This comment has been minimized.

@SimonHooker
Copy link

SimonHooker commented Jan 17, 2020

As a stop-gap, I copied the entirety of the lib/protocol/constants/ssl_profiles.js file on this PR into a new file. I then updated the connection parameters as follows :

params_for_connection.ssl = 'Amazon RDS';

... becomes ...

const MysqlSSLProfiles = require( './path/to/imported/ssl_profiles.js' );
...
params_for_connection.ssl = MysqlSSLProfiles[ 'Amazon RDS' ];

Hopefully this is of help to anyone that needs to get a band-aid up ahead of this being merged.

@dougwilson
Copy link
Member

dougwilson commented Jan 17, 2020

I am heading to sleep now, but I wanted to provide an update on this issue (reminded by the post above): I will have a new minor version with this change published on either Jan 18 or 19 (depending on the time zone you are in).

Edit: I just resumed work to get this done, only to realize I had the dates off by one day, as they should have matched Sat/Sun, so updated them.

Edit: apologies 🙏 this weekend where I live has been having power failures, so I got a lot less done than I anticipated. Power is back and running, though it is about time for me to go to sleep, but no worries as Jan 20 is a holiday for me, and so I will be spending the morning getting a new mysql version published.

@CovertLeopard

This comment has been minimized.

@dougwilson

This comment has been minimized.

@brettneese
Copy link

@dougwilson thanks for your transparency and keep on keeping on 👍 😄

do let us know if you need any help!!!

@CovertLeopard
Copy link

CovertLeopard commented Jan 20, 2020 via email

@dougwilson
Copy link
Member

dougwilson commented Jan 20, 2020

This profile is just a preset config given to the ssl option of this module... it does nothing you cannot already do. All it is doing is translating the string "Amazon RDS" into the object that is in the ssl_profiles.js ... there is no requirement to wait for this unless you have to just see ssl: "Amazon RDS" in your config.

@dougwilson
Copy link
Member

hoping I could at least try the new RDS profile implemented by the developers of this module, just in case my implementation of manually specifying the RDS root ca cert is flawed

The time to try it out to make sure your issue is resolved is now, prior to merging and releasing... the changes are in the "Changes" tab above, as everything is open source. If you're not sure these changes will actually work for your issue, ideally if you can find out before I release that will have the best turn around time. That said, the date you gave is over a week away and I promised Jan 20, so what specific time on Jan 20 I'd be surprised that would have a bearing on such a long lead time?

@CovertLeopard
Copy link

CovertLeopard commented Jan 20, 2020 via email

@berzniz
Copy link

berzniz commented Jan 20, 2020

There is an npm module if you need it to bridge this gap: https://github.com/toriihq/mysql-rds-ca — it's been tested on production in our various servers, with RDS and Aurora.

I think going forward it would make sense to keep that part outside of this package.

@dougwilson
Copy link
Member

I think going forward it would make sense to keep that part outside of this package.

It is definitely on my mind for when the next major comes up. There are a lot of benefits, from just general decoupling like users who never use Amazon RDS endpoints do not need to have all the certs loaded up, to management where it could be in the hands of folks who use Amazon RDS every day, which would help especially around things like #2276 .

After all these years, the only profile that has never been has been Amazon RDS, so it's not like there are many to choose from... and it seems like the "newer" Amazon MySQL system presents certs that are trusted by Node.js itself by default, and I would guess Amazon RDS would eventually move to that making the need for a profile obsolete at some point.

Plus you can use that module with mysql2 as well, making just one place to keep up-to-date for everyone, nice!

@dougwilson
Copy link
Member

Hi everyone, as you may have gotten the notice, this PR has been merged. Sorry for the delay, a lot of time was spent waiting for CI to complete on the various changes. The new version is not published just yet, as I have learned my lesson in the past when publishing a release and immediately going to sleep :) I will be publishing in approx 8 hours from now, though you are welcome to test out master before hand. I will also be around after that in case there is a regression or other issue in the new version to address promptly.

@dougwilson
Copy link
Member

Did anyone in this thread test this PR before I merged and released it? Can someone chime in on #2292 ?

@SimonHooker
Copy link

SimonHooker commented Jan 22, 2020

@dougwilson as noted in my comment, I manually replicated this PR within my source code and confirmed it operational against a ca-2019 RDS instance using the steps detailed, which I believe should be fundamentally identical to the PR.

However there are additional CA certificates for different access points. This PR only actually included the global one. The additional APs probably also need merging, and then later on a cleanup after March 2020 can remove the 2015-2020 CA certificates as they will be useless att hat point.

@jthele
Copy link
Contributor

jthele commented Jan 22, 2020

@SimonHooker: What version of MySQL was your RDS instance running? Perhaps there may be different behavior between older and newer server versions.

@SimonHooker
Copy link

SimonHooker commented Jan 22, 2020

@jthele MySQL 5.7.26

The certificate added in #2280 was the global CA, however there are intermediates though not sure why it would work for me without the intermediate, but others would require it. It sounds like this may be related to using AWS GovCloud or similar setup, detailed on https://docs.aws.amazon.com/AmazonRDS/latest/UserGuide/UsingWithRDS.SSL.html

For consistency #2292 probably should be merged in also

@jthele
Copy link
Contributor

jthele commented Jan 22, 2020

I am not using GovCloud. I am on MySQL 5.6.37.

@dougwilson
Copy link
Member

Maybe it is a difference in Node.js version being used?

@CovertLeopard
Copy link

Did anyone in this thread test this PR before I merged and released it? Can someone chime in on #2292 ?

I'm using it with AWS Lambda Functions. v2.18.0 works with nodejs8.10, nodejs10.x and nodejs12.x lambda runtimes in the US-East-1 region when connecting to an Aurora MySQL RDS instance on MySQL 5.7.12 engine. It's a standard account, not GovCloud or any other specialty. I'll add this comment to #2292 as well.

@ewarren
Copy link

ewarren commented Jan 29, 2020

Hello,
We have moved from this implementation:

dialectOptions: {
        ssl: 'Amazon RDS'
}

To this instead:

var Sequelize = require('sequelize');
const fs = require('fs');
const rdsCa = fs.readFileSync('rds-combined-ca-bundle.pem');

// configuration
var sequelize = new Sequelize(process.env.DB_DATABASE, process.env.DB_USER, process.env.DB_PASSWORD, {
    dialect: 'mysql',
    dialectOptions: {
        ssl: {
            rejectUnauthorized: true,
            ca: [rdsCa]
        }
    },	

However, now we have to build the .pem file into our zip file that we deploy on our EC2 instances.
Is that what is recommended? We never did this before, as just specifiying 'Amazon RDS' as the ssl string under dialectOptions seemed to used to pull the contents of the .pem file automatically.

Is there any risk to us bundling the .pem file into our deployable zip file?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.