AaronBurchfield / CloudFront-Middleware

Securely access a munki repo with Amazon CloudFront
36 stars 9 forks source link

Pull cert from preference #5

Closed clburlison closed 6 years ago

clburlison commented 6 years ago

@AaronBurchfield would you accept a pull request to pull the cloudfront private cert via a preference? This would allow rotation of the cert via an MDM and no need to drop the cert on the filesystem.

erikng commented 6 years ago

This would be a sweet idea. Could either be a data payload or a base64 encoded string.

My recommendation would be the latter so tools that have issues with data payloads could also manage this preference and not be limited to MDM (say chef).

AaronBurchfield commented 6 years ago

This sounds like a great improvement. I agree with Erik, we'd want to be able to install this with puppet as well as MDM.

A PR would be fine, let me know if I can do anything to help! đź‘Ť

clburlison commented 6 years ago

Good to hear. I already have the code written so it shouldn’t be hard to get this merged. I wrote the the proper way using a data type since that’s the most logical for this type of object. As for Erik’s comment I understand why he wants a string type.

Technically you can push the profile via puppet or chef with a data object but for easy of use I don’t have an issue adding the code for a string type.

Now for the hard part, naming keys. My original thought is a cloudfront_certificate key that was a data type. We could overload that key and accept both base64 encoded string and data type however my preference would be to use two preference keys. Maybe cloudfront_str_certificate and cloudfront_data_certificate??

Then in ththe middleware script check for those keys first and then fallback to the on disk certificate?

I’m not in love with the key names so feedback please. I do want to get this implemented properly from the start so we don’t have to rename keys in the future.

erikng commented 6 years ago

I think those keys are fine.

Only thing is what is the behavior if someone stupidly uses both keys and for whatever reason they are different certs?

What if it was: cloudfront_certificate cloudfront_certificate_type

This way we parse the type first and then you know how to handle the other key. This would solve the potential of someone doing something stupid and be very explicit as to what should be expected.

Thanks, Erik Gomez


From: Clayton Burlison notifications@github.com Sent: Thursday, August 9, 2018 9:27:15 PM To: AaronBurchfield/CloudFront-Middleware Cc: Erik Gomez; Comment Subject: Re: [AaronBurchfield/CloudFront-Middleware] Pull cert from preference (#5)

Good to hear. I already have the code written so it shouldn’t be hard to get this merged. I wrote the the proper way using a data type since that’s the most logical for this type of object. As for Erik’s comment I understand why he wants a string type.

Technically you can push the profile via puppet or chef with a data object but for easy of use I don’t have an issue adding the code for a string type.

Now for the hard part, naming keys. My original thought is a cloudfront_certificate key that was a data type. We could overload that key and accept both base64 encoded string and data type however my preference would be to use two preference keys. Maybe cloudfront_str_certificate and cloudfront_data_certificate??

Then in ththe middleware script check for those keys first and then fallback to the on disk certificate?

I’m not in love with the key names so feedback please. I do want to get this implemented properly from the start so we don’t have to rename keys in the future.

— You are receiving this because you commented. Reply to this email directly, view it on GitHubhttps://github.com/AaronBurchfield/CloudFront-Middleware/issues/5#issuecomment-411956504, or mute the threadhttps://github.com/notifications/unsubscribe-auth/AFa_GPnWnS-5k9vRel5iUo46nh_GBNyQks5uPO-DgaJpZM4V2SH-.

clburlison commented 6 years ago

I like cloudfront_certificate cloudfront_certificate_type however if we went that route might as well remove the “type” key and do a type check in python. That would be overloading the same key with two different types. I’m still not 100% sure why that feels like a bad idea but maybe I’m wrong. If no other feedback comes back I’ll try to put this together over the weekend for review.

AaronBurchfield commented 6 years ago

A single key called cloudfront_certificate seems fine even with multiple types. I don't think it's unreasonable to expect the admin to format the certificate and set the preferences correctly for something like this.

clburlison commented 6 years ago

PR created please take a look.