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.

Sign up for GitHub

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

Jump to bottom

Fix elliptic curve certs and add a unit test #243

Merged
merged 3 commits into from
Feb 27, 2019
Merged

Fix elliptic curve certs and add a unit test #243

merged 3 commits into from
Feb 27, 2019

Conversation

brendandburns
Copy link
Contributor

Fixes #242

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Feb 14, 2019
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: brendandburns

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 14, 2019
@k8s-ci-robot k8s-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Feb 14, 2019
@brendandburns brendandburns requested review from tg123 and itowlson and removed request for krabhishek8260 February 14, 2019 02:50
@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Feb 14, 2019
@brendandburns brendandburns reopened this Feb 14, 2019
itowlson
itowlson suggested changes Feb 14, 2019
View reviewed changes
Copy link
Contributor

@itowlson itowlson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The actual code looks fine! ...but I have cosmetic nitpicks with all the other files...

tests/KubernetesClient.Tests/KubernetesClientConfigurationTests.cs Outdated
/// <param name="context"></param>
[Theory]
[InlineData("craftsman-context")]
public void ContextElpiticKey(string context)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo

Suggested change
public void ContextElpiticKey(string context)
public void ContextEllipticKey(string context)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done.

tests/KubernetesClient.Tests/assets/kubeconfig.yml Outdated
@@ -51,3 +56,7 @@ users:
user:
client-certificate-data: LS0tLS1CRUdJTiBDRVJUSUZJQ0FURS0tLS0tCk1JSURrVENDQW5tZ0F3SUJBZ0lVTzlVTkdpbHhmSHpMbVJXcWU2dVMyRVZ1NVhVd0RRWUpLb1pJaHZjTkFRRUwKQlFBd1FURUxNQWtHQTFVRUJoTUNWVk14RURBT0JnTlZCQWdUQjFKbFpHMXZibVF4Q3pBSkJnTlZCQWNUQWxkQgpNUk13RVFZRFZRUURFd3ByZFdKbGNtNWxkR1Z6TUNBWERURTRNVEl3T0RBNU5URXdNRm9ZRHpJeE1UZ3hNVEUwCk1EazFNVEF3V2pCQk1Rc3dDUVlEVlFRR0V3SlZVekVRTUE0R0ExVUVDQk1IVW1Wa2JXOXVaREVMTUFrR0ExVUUKQnhNQ1YwRXhFekFSQmdOVkJBTVRDbXQxWW1WeWJtVjBaWE13Z2dFaU1BMEdDU3FHU0liM0RRRUJBUVVBQTRJQgpEd0F3Z2dFS0FvSUJBUUM4WkVRS296SE8zOExmRzRpcHFYWnRLTzNxSENjL1Z1WjUxWnZSeWJuTnpCYU5iMk9oClM0cU1nNDF6cFdzL3dMQnk4ZndPclpoOHpHb28wbHllQXVCSlBPWFc5SmswMmhOc1Y3ZHBqYXBZWFMrSXJvN04KcUxhWDB5amQxWWllSWFlb3NtV2xib2ZpcDVzS3dzVUI3bGREeXJpQklBYTEwNlhhTng5ZG82UEh1TDNibStldwpiaWRoRTlNUFRHY2V5WW9rZ3pNbGppanordk0yUnN5Z05ncmR4VDhROC9GRER2ZndTRmZUaEZlYXIzckQvRkJGCmVYb0Y5MHp6cG1aZlphTDlyZ2NjQ0pzQ2JSZlpiellVVERRVHo2dStaUVhUdGlrbVMvQ3d2d0hXa0w3bGhQNXYKQU0yVFpETEJhQXQwOU14dGlORFNWaFFTWVR2QWpKRmU4SzJoQWdNQkFBR2pmekI5TUE0R0ExVWREd0VCL3dRRQpBd0lGb0RBZEJnTlZIU1VFRmpBVUJnZ3JCZ0VGQlFjREFRWUlLd1lCQlFVSEF3SXdEQVlEVlIwVEFRSC9CQUl3CkFEQWRCZ05WSFE0RUZnUVVuQmo0T3Y3MnFlWWJ5YjVYSHJma1pkazd6VUF3SHdZRFZSMGpCQmd3Rm9BVUxwemQKRlplYkR6N0RmZUNkZkJqNmNkUWJmNk13RFFZSktvWklodmNOQVFFTEJRQURnZ0VCQU1CL1RDaElEbTNkeDA4SApJeXlFS2dYUHh1d3A1cTB5QkFoT0pUS1JOVXNpayt6ZVUyUGpibnV0M3B1WnBENkZoTGFKQUZPbTdsMHhkNU1ZCkFZOVloSy9wa1U3Rmw1TFg0MitxQzRqK05JdGhBbFM5clNtNkRvNlN1b3JRcjdNajErY0syTjFkZHBtRWZya3kKZXZnMm02aFB5YTFlV0hNanVCd250bkJmV2EzWHBWMWVGZVBvd21zY3R6UmRJYUh3WDM3Yit3c2JmbkppczRvTgo2bHk0THBQb2xtYld3ZTRnaEtPWXNuL3lMT3FrUGJMZVpLeU5yaWJpSGhSQTM0NUVHUGJleXNmSlNIWWFqMnBaCmE0VmRTRFhnT2JlM3Vtdk9keXVEaGl5RFJ3TFozbVJLWlpIR0lzZC9ORzRncWo3a0gzV2N2Wm5mSkVHMXY2cG0KQTZFYTJiRT0KLS0tLS1FTkQgQ0VSVElGSUNBVEUtLS0tLQo=
client-key-data: LS0tLS1CRUdJTiBSU0EgUFJJVkFURSBLRVktLS0tLQpNSUlFb3dJQkFBS0NBUUVBdkdSRUNxTXh6dC9DM3h1SXFhbDJiU2p0Nmh3blAxYm1lZFdiMGNtNXpjd1dqVzlqCm9VdUtqSU9OYzZWclA4Q3djdkg4RHEyWWZNeHFLTkpjbmdMZ1NUemwxdlNaTk5vVGJGZTNhWTJxV0YwdmlLNk8KemFpMmw5TW8zZFdJbmlHbnFMSmxwVzZINHFlYkNzTEZBZTVYUThxNGdTQUd0ZE9sMmpjZlhhT2p4N2k5MjV2bgpzRzRuWVJQVEQweG5Ic21LSklNekpZNG84L3J6TmtiTW9EWUszY1UvRVBQeFF3NzM4RWhYMDRSWG1xOTZ3L3hRClJYbDZCZmRNODZabVgyV2kvYTRISEFpYkFtMFgyVzgyRkV3MEU4K3J2bVVGMDdZcEprdndzTDhCMXBDKzVZVCsKYndETmsyUXl3V2dMZFBUTWJZalEwbFlVRW1FN3dJeVJYdkN0b1FJREFRQUJBb0lCQUNCYk9EUjdndnA5QkFNOQp2Mk1rYitxZnRQMFlpTVVnTDhXTklvNE5qNVFCRVg2Sk94dGcxaEw4SlRkUG1mUUJMRTBSc3JEeXI5WC9aZHhOCkJRcytnemNROW9qTXllT0I4UVFTckxXOFZ4MkdJN3ZkL3pqaldUa0tVMktHWWtpR2p6MHlKck1iSU11VTdkUVQKVDdMZE5LKzRDYWhqejhNNjdxbGova2NlNitwSlRLdHZKR2tNSDRYUXJyMFRmU3hLMmEwUUNmNkZwSXp2OFFEMApISE9HbFJaWlBYK1dXYVA2UFpFU3JSZG1vbmFKaTJlRG03b1Y1WU1DMExWTHJzMVprZS9FT01CODZYOVlraGJUCmJsRkxRZjlNR1hTdWtGeW1MSnFSd01DUjJxZytKSXpEU1BuYzkzbjNwcHl4Nys3UkhFeXZNMFVNOVhyeHQrNHcKUkd3ZVZlRUNnWUVBeElkaFRCdW4rRk52YkVhSHZUdlpzNkVCa1lJUXUyV0NDR3dWN1lPbTZzc0hzczJBaldrVgprQ0pMajhZMTFpMHd0Y21yOG9DcE1EN0FDRWxMSWFEK3RubW5CTzlkQ2NnTzJoNXFCSkY1UytJeFlGcVZ5ZVBsCnptQkE2VzFvdzB2Y1hsYmkrVUg4SWF3WnNLaFlMaklscmdmQ3YwaU9LNzBYaEF3YzZ6Vlk4TjhDZ1lFQTlXYUUKMklkNjJEaGxLWVRaNW5yY3g1aVN3WU1jaitMTE5FYTFZMlZFNEhZZktvbjFkRnNSa2NKV1YyRVpoRGlqcVIyaQpXZEdEVldiMVVaVEpiM3BZWW1NakJFQXhFbEJXTSt0SzhialRqMWhxbXJPQWhLU0ZYUjg4RkNEanVkcStSbmo4CklyZFlWanNCdDA5RkVkT2RqdVFseGgyRVdQWDNPdVFhRXd5WnNYOENnWUJzekNtZ0VadHVqUG9kTGZxTlZ5blIKR0t3ZW1xdWFvcnBXNFVkT1l0aXdHTS9kTzRrVVAvMlErbnRzVDZXVU9SWkRQUzgwbytlRjd1Y3VieXpwcEEvKwpndUJraWdLdW5KTWtTendUNVZrS0dtR05YdmlYZU5QSzZWeG1IWXltdVVONDhvN2F3SjNOSWxKaWl2K3VLMUxTCndqY2M0QlRjditUWjFEN2FNNEZXYndLQmdHY1MyNE96VEJiYmdTb3lRZS83OVJYazhPZFU4YjlCN0VZVjJRUloKdWRkcDVlZFJNUWJoWlh6S21zZHk0bXZWK25BRElYa0dkbHA5dDFhLzN1ZnpCSUsyenpOdTN1MnBUcnZaL1kyUQpLMVJQTjkrb3U3ZDYvd1ZCSkZQMENKSzgzU1R1bGtEaXI3andhZVViNTQvNFNYcUdPNU4rUEdPOVZFMnBGNGFlCnlVTnpBb0dCQUptMU1vcFQ4N0llelBmQ010ck51cXVqQjFpdStvb1lIYm9IbGJXZUNxS3RSRnNYMWhXOWpTY1UKQ0ZRanhSVCtPcENVWndjbDdkY2xsTUlxQTRWQ1NmUUFRMW9CMU41WENRWE8xVFlXOHU1K1BRTUdUcFEvQlVEawp6WmRJZzZqQkFXd3R6YkNzKzRjVkFoclN3cDRBSXFvcndTZU1qRmdsTmNoNzgxMEF6emNJCi0tLS0tRU5EIFJTQSBQUklWQVRFIEtFWS0tLS0tCg==
- name: purple-user
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I hope I'm not being humourless (the sure sign that I am being humourless), but I think we should aim to name test data etc. after its role in the test e.g. elliptic-key-user rather than purple - this makes it much easier to understand the difference between e.g. purple-user and red-user without having to search through the tests to see where they are used and reverse-engineer what their significance is.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done (I was sticking to the pre-established theme, which I didn't start :)

tests/KubernetesClient.Tests/assets/ec-client.key Outdated
@@ -0,0 +1,29 @@
-----BEGIN EC PARAMETERS-----
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would consider renaming this file e.g. elliptic-client.key so that it is easier to know what it's for from the filename without having to look at how it is used or search up the Git commit message.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done.

tests/KubernetesClient.Tests/KubernetesClientConfigurationTests.cs Outdated
@@ -54,6 +54,20 @@ public void ContextCertificate(string context, string clientCert, string clientC
Assert.Equal(cfg.ClientKeyFilePath, clientCertKey);
}

/// <summary>
/// Checks for loading of eliptical curve keys
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo

Suggested change
/// Checks for loading of eliptical curve keys
/// Checks for loading of elliptical curve keys

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done.

@itowlson itowlson changed the title Fix a elipitic curve certs and add a unit test. Fix elliptic curve certs and add a unit test Feb 14, 2019
@brendandburns
Copy link
Contributor Author

@itowlson comments addressed (and test fixed for CI) please re-check.

Thanks!

@itowlson
Copy link
Contributor

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 14, 2019
@itowlson
Copy link
Contributor

CI has picked up what may be a portability issue:

Failed   k8s.Tests.KubernetesClientConfigurationTests.ContextEllipticKey(context: "elliptic-context")
Error Message:
 Interop+AppleCrypto+AppleCommonCryptoCryptographicException : Unknown format in import.
Stack Trace:
   at Interop.AppleCrypto.X509ImportCertificate(Byte[] bytes, X509ContentType contentType, SafePasswordHandle importPassword, SafeKeychainHandle keychain, Boolean exportable, SafeSecIdentityHandle& identityHandle)
   at Internal.Cryptography.Pal.CertificatePal.FromBlob(Byte[] rawData, SafePasswordHandle password, X509KeyStorageFlags keyStorageFlags)
   at System.Security.Cryptography.X509Certificates.X509Certificate..ctor(Byte[] data)
   at System.Security.Cryptography.X509Certificates.X509Certificate2..ctor(Byte[] rawData)
...

Is it expected/acceptable that elliptic keys would not work on Mac? If so could just turn off the test in Mac environments?

@brendandburns
Copy link
Contributor Author

Maybe it's this specific EC key? I'll try to generate one on a mac and see if it works...

@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 21, 2019
@brendandburns
Copy link
Contributor Author

@itowlson I managed to create an elliptic key that made both the mac and linux happy...

@brendandburns
Copy link
Contributor Author

@itowlson friendly ping on this one (and the dotnet version update PR too :)

thanks!

@itowlson
Copy link
Contributor

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 27, 2019
@k8s-ci-robot k8s-ci-robot merged commit 948d78c into kubernetes-client:master Feb 27, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tg123 tg123 tg123 approved these changes

@itowlson itowlson itowlson approved these changes

Assignees

@itowlson itowlson

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@brendandburns @k8s-ci-robot @itowlson @tg123