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

[YW] Handle invalid certs/keys correctly #5594

Closed
iSignal opened this issue Sep 4, 2020 · 3 comments · Fixed by #7264
Closed

[YW] Handle invalid certs/keys correctly #5594

iSignal opened this issue Sep 4, 2020 · 3 comments · Fixed by #7264
Assignees
Labels
area/platform Yugabyte Platform good first issue This is a good issue to start contributing! team/platform-hi Tickets handled by HashedIn team
Projects
Milestone

Comments

@iSignal
Copy link
Contributor

iSignal commented Sep 4, 2020

  1. Creating a universe with an invalid certificate key/crt leads to the following error trace - however the universe itself shows up as "Ready" with no errors. Deleting this kind of a universe may also fail because it is not full set up.

  2. A similar error happens if one goes to the Certificates list and clicks Actions -> Get Client certificate from the dropdown. One issue there is that there is no way to remove the invalid cert from the list (because we do not allow removing any certs once they are added).

2020-09-04 05:22:51.286 INFO  UniverseController.java:465 [application-akka.actor.default-dispatcher-11] Created universe 9f15fa61-5e3d-47b3-8621-85af2e93636b : sanketh-test-1.
2020-09-04 05:22:51.289 INFO  UniverseController.java:471 [application-akka.actor.default-dispatcher-11] Added universe 9f15fa61-5e3d-47b3-8621-85af2e93636b : sanketh-test-1 for customer [1].
2020-09-04 05:22:51.607 ERROR CertificateHelper.java:183 [application-akka.actor.default-dispatcher-11] Unable to create client CA for username yugabyte: {}
java.security.spec.InvalidKeySpecException: java.security.InvalidKeyException: invalid key format
	at sun.security.rsa.RSAKeyFactory.engineGeneratePrivate(RSAKeyFactory.java:252)
	at java.security.KeyFactory.generatePrivate(KeyFactory.java:372)
	at com.yugabyte.yw.common.CertificateHelper.createClientCertificate(CertificateHelper.java:181)
	at com.yugabyte.yw.controllers.UniverseController.create(UniverseController.java:494)
	at v1.Routes$$anonfun$routes$1$$anonfun$applyOrElse$67$$anonfun$apply$67.apply(Routes.scala:3112)
	at v1.Routes$$anonfun$routes$1$$anonfun$applyOrElse$67$$anonfun$apply$67.apply(Routes.scala:3112)
	at play.core.routing.HandlerInvokerFactory$$anon$3.resultCall(HandlerInvoker.scala:143)
	at play.core.routing.HandlerInvokerFactory$$anon$3.resultCall(HandlerInvoker.scala:142)
	at play.core.routing.HandlerInvokerFactory$JavaActionInvokerFactory$$anon$8$$anon$2$$anon$1.invocation(HandlerInvoker.scala:116)
	at play.core.j.JavaAction$$anon$1.call(JavaAction.scala:112)
	at play.http.DefaultActionCreator$1.call(DefaultActionCreator.java:33)
	at com.yugabyte.yw.controllers.TokenAuthenticator.call(TokenAuthenticator.java:107)
	at play.core.j.JavaAction$$anonfun$9.apply(JavaAction.scala:171)
	at play.core.j.JavaAction$$anonfun$9.apply(JavaAction.scala:171)
	at scala.concurrent.impl.Future$PromiseCompletingRunnable.liftedTree1$1(Future.scala:24)
	at scala.concurrent.impl.Future$PromiseCompletingRunnable.run(Future.scala:24)
	at play.core.j.HttpExecutionContext$$anon$2.run(HttpExecutionContext.scala:65)
	at play.api.libs.streams.Execution$trampoline$.execute(Execution.scala:72)
	at play.core.j.HttpExecutionContext.execute(HttpExecutionContext.scala:57)
	at scala.concurrent.impl.Future$.apply(Future.scala:31)
	at scala.concurrent.Future$.apply(Future.scala:492)
	at play.core.j.JavaAction.apply(JavaAction.scala:171)
	at play.api.mvc.Action$$anonfun$apply$2.apply(Action.scala:98)
	at play.api.mvc.Action$$anonfun$apply$2.apply(Action.scala:91)
	at scala.concurrent.Future$$anonfun$flatMap$1.apply(Future.scala:251)
	at scala.concurrent.Future$$anonfun$flatMap$1.apply(Future.scala:249)
	at scala.concurrent.impl.CallbackRunnable.run(Promise.scala:32)
	at akka.dispatch.BatchingExecutor$AbstractBatch.processBatch(BatchingExecutor.scala:55)
	at akka.dispatch.BatchingExecutor$BlockableBatch$$anonfun$run$1.apply$mcV$sp(BatchingExecutor.scala:92)
	at akka.dispatch.BatchingExecutor$BlockableBatch$$anonfun$run$1.apply(BatchingExecutor.scala:92)
	at akka.dispatch.BatchingExecutor$BlockableBatch$$anonfun$run$1.apply(BatchingExecutor.scala:92)
	at scala.concurrent.BlockContext$.withBlockContext(BlockContext.scala:72)
	at akka.dispatch.BatchingExecutor$BlockableBatch.run(BatchingExecutor.scala:91)
	at akka.dispatch.TaskInvocation.run(AbstractDispatcher.scala:41)
	at akka.dispatch.ForkJoinExecutorConfigurator$AkkaForkJoinTask.exec(ForkJoinExecutorConfigurator.scala:49)
	at akka.dispatch.forkjoin.ForkJoinTask.doExec(ForkJoinTask.java:260)
	at akka.dispatch.forkjoin.ForkJoinPool$WorkQueue.runTask(ForkJoinPool.java:1339)
	at akka.dispatch.forkjoin.ForkJoinPool.runWorker(ForkJoinPool.java:1979)
	at akka.dispatch.forkjoin.ForkJoinWorkerThread.run(ForkJoinWorkerThread.java:107)
Caused by: java.security.InvalidKeyException: invalid key format
	at sun.security.pkcs.PKCS8Key.decode(PKCS8Key.java:331)
	at sun.security.pkcs.PKCS8Key.decode(PKCS8Key.java:356)
	at sun.security.rsa.RSAPrivateCrtKeyImpl.<init>(RSAPrivateCrtKeyImpl.java:130)
	at sun.security.rsa.RSAPrivateCrtKeyImpl.newKey(RSAPrivateCrtKeyImpl.java:80)
	at sun.security.rsa.RSAKeyFactory.generatePrivate(RSAKeyFactory.java:357)
	at sun.security.rsa.RSAKeyFactory.engineGeneratePrivate(RSAKeyFactory.java:248)
	... 38 common frames omitted
2020-09-04 05:22:51.607 ERROR UniverseController.java:557 [application-akka.actor.default-dispatcher-11] Error creating universe
java.lang.RuntimeException: Could not create client cert.
	at com.yugabyte.yw.common.CertificateHelper.createClientCertificate(CertificateHelper.java:184)
	at com.yugabyte.yw.controllers.UniverseController.create(UniverseController.java:494)
	at v1.Routes$$anonfun$routes$1$$anonfun$applyOrElse$67$$anonfun$apply$67.apply(Routes.scala:3112)
	at v1.Routes$$anonfun$routes$1$$anonfun$applyOrElse$67$$anonfun$apply$67.apply(Routes.scala:3112)
	at play.core.routing.HandlerInvokerFactory$$anon$3.resultCall(HandlerInvoker.scala:143)
	at play.core.routing.HandlerInvokerFactory$$anon$3.resultCall(HandlerInvoker.scala:142)
	at play.core.routing.HandlerInvokerFactory$JavaActionInvokerFactory$$anon$8$$anon$2$$anon$1.invocation(HandlerInvoker.scala:116)
	at play.core.j.JavaAction$$anon$1.call(JavaAction.scala:112)
	at play.http.DefaultActionCreator$1.call(DefaultActionCreator.java:33)
	at com.yugabyte.yw.controllers.TokenAuthenticator.call(TokenAuthenticator.java:107)
	at play.core.j.JavaAction$$anonfun$9.apply(JavaAction.scala:171)
	at play.core.j.JavaAction$$anonfun$9.apply(JavaAction.scala:171)
	at scala.concurrent.impl.Future$PromiseCompletingRunnable.liftedTree1$1(Future.scala:24)
	at scala.concurrent.impl.Future$PromiseCompletingRunnable.run(Future.scala:24)
	at play.core.j.HttpExecutionContext$$anon$2.run(HttpExecutionContext.scala:65)
	at play.api.libs.streams.Execution$trampoline$.execute(Execution.scala:72)
	at play.core.j.HttpExecutionContext.execute(HttpExecutionContext.scala:57)
	at scala.concurrent.impl.Future$.apply(Future.scala:31)
	at scala.concurrent.Future$.apply(Future.scala:492)
	at play.core.j.JavaAction.apply(JavaAction.scala:171)
	at play.api.mvc.Action$$anonfun$apply$2.apply(Action.scala:98)
	at play.api.mvc.Action$$anonfun$apply$2.apply(Action.scala:91)
	at scala.concurrent.Future$$anonfun$flatMap$1.apply(Future.scala:251)
	at scala.concurrent.Future$$anonfun$flatMap$1.apply(Future.scala:249)
	at scala.concurrent.impl.CallbackRunnable.run(Promise.scala:32)
	at akka.dispatch.BatchingExecutor$AbstractBatch.processBatch(BatchingExecutor.scala:55)
	at akka.dispatch.BatchingExecutor$BlockableBatch$$anonfun$run$1.apply$mcV$sp(BatchingExecutor.scala:92)
	at akka.dispatch.BatchingExecutor$BlockableBatch$$anonfun$run$1.apply(BatchingExecutor.scala:92)
	at akka.dispatch.BatchingExecutor$BlockableBatch$$anonfun$run$1.apply(BatchingExecutor.scala:92)
	at scala.concurrent.BlockContext$.withBlockContext(BlockContext.scala:72)
	at akka.dispatch.BatchingExecutor$BlockableBatch.run(BatchingExecutor.scala:91)
	at akka.dispatch.TaskInvocation.run(AbstractDispatcher.scala:41)
	at akka.dispatch.ForkJoinExecutorConfigurator$AkkaForkJoinTask.exec(ForkJoinExecutorConfigurator.scala:49)
	at akka.dispatch.forkjoin.ForkJoinTask.doExec(ForkJoinTask.java:260)
	at akka.dispatch.forkjoin.ForkJoinPool$WorkQueue.runTask(ForkJoinPool.java:1339)
	at akka.dispatch.forkjoin.ForkJoinPool.runWorker(ForkJoinPool.java:1979)
	at akka.dispatch.forkjoin.ForkJoinWorkerThread.run(ForkJoinWorkerThread.java:107)
2020-09-04 05:22:51.607 ERROR ApiResponse.java:21 [application-akka.actor.default-dispatcher-11] Hit error 500, message: {"error":"Could not create client cert."}
@iSignal iSignal added the area/platform Yugabyte Platform label Sep 4, 2020
@streddy-yb
Copy link
Contributor

Hi @iSignal - Should we fail the upload cert action if the cert is invalid? Can you handle this as part of TLS workflow improvements? thanks

@streddy-yb streddy-yb added this to To do in Platform Sep 8, 2020
@iSignal iSignal changed the title Creating a universe with invalid key/cert does not result in error state [YW] Handle invalid certs/keys correctly Dec 9, 2020
@iSignal
Copy link
Contributor Author

iSignal commented Dec 9, 2020

We could do two improvements here:

  1. Verify the cert at upload time itself
  2. The failure to create client certs is somehow not bubbling up to the create universe task flow. We also need to make sure the universe creation fails properly and that it can be then be deleted properly.

For the future, we could also consider allowing deletion of certs that do not have any univs associated with them.

@streddy-yb streddy-yb added this to the 2.5.x milestone Dec 9, 2020
@streddy-yb streddy-yb added the team/platform-hi Tickets handled by HashedIn team label Dec 9, 2020
@streddy-yb streddy-yb added the good first issue This is a good issue to start contributing! label Dec 10, 2020
@jaydeepkumara jaydeepkumara self-assigned this Dec 10, 2020
@jaydeepkumara jaydeepkumara moved this from To do to In progress in Platform Dec 23, 2020
jitendra-12113 pushed a commit that referenced this issue Jan 4, 2021
Heading:
[5594][Platform] Handle invalid certs/keys correctly
Description:
We wanted to verify the cert at upload time itself.
Testing:
When we try to upload selfSigned cert, we were able to upload any
garbage files. Now with this fix, we have handled this scenrio. User
won't be able to upload garbage file. Incase of self signed cert upload
workflow, We are matching the modulus of
cert RSA Public key with the modulus of its private key if its matched
then it means certs are valid otheriwse Invalid.
For CA Cert, we are just validating the Cert format.
Added the unit Tests as well.
@jitendra-12113
Copy link
Contributor

jitendra-12113 commented Jan 4, 2021

Back end changes are done. Please review.
#6780

PS: UI changes need to be done accordingly.
cc: @gaurav061

@jitendra-12113 jitendra-12113 linked a pull request Jan 4, 2021 that will close this issue
@jitendra-12113 jitendra-12113 moved this from In progress to In Review in Platform Jan 4, 2021
jitendra-12113 pushed a commit that referenced this issue Jan 5, 2021
Heading:
[5594][Platform] Handle invalid certs/keys correctly
Description:
We wanted to verify the cert at upload time itself.
Testing:
When we try to upload selfSigned cert, we were able to upload any
garbage files. Now with this fix, we have handled this scenrio. User
won't be able to upload garbage file. Incase of self signed cert upload
workflow, We are matching the modulus of
cert RSA Public key with the modulus of its private key if its matched
then it means certs are valid otheriwse Invalid.
For CA Cert, we are just validating the Cert format.
Added the unit Tests as well.
@jitendra-12113 jitendra-12113 linked a pull request Feb 8, 2021 that will close this issue
jitendra-12113 added a commit that referenced this issue Feb 17, 2021
* Heading:
[5594][Platform] Handle invalid certs/keys correctly
Description:
We wanted to verify the cert at upload time itself.
Testing:
When we try to upload selfSigned cert, we were able to upload any
garbage files. Now with this fix, we have handled this scenrio. User
won't be able to upload garbage file. Incase of self signed cert upload
workflow, We are matching the modulus of
cert RSA Public key with the modulus of its private key if its matched
then it means certs are valid otheriwse Invalid.
For CA Cert, we are just validating the Cert format.
Added the unit Tests as well.

(cherry picked from commit 5f8d39a)

* Heading:
[5594][Platform] Handle invalid certs/keys correctly
Description:
We wanted to verify the cert at upload time itself.
Testing:
When we try to upload selfSigned cert, we were able to upload any
garbage files. Now with this fix, we have handled this scenrio. User
won't be able to upload garbage file. Incase of self signed cert upload
workflow, We are matching the modulus of
cert RSA Public key with the modulus of its private key if its matched
then it means certs are valid otheriwse Invalid.
For CA Cert, we are just validating the Cert format.
Added the unit Tests as well.

(cherry picked from commit 5f8d39a)

Co-authored-by: jitendra-12113 <jitedra.kumar@hashedin.com>
@jitendra-12113 jitendra-12113 moved this from In Review to Needs QA/Docs in Platform Feb 17, 2021
polarweasel pushed a commit to lizayugabyte/yugabyte-db that referenced this issue Mar 9, 2021
…yte#7264)

* Heading:
[5594][Platform] Handle invalid certs/keys correctly
Description:
We wanted to verify the cert at upload time itself.
Testing:
When we try to upload selfSigned cert, we were able to upload any
garbage files. Now with this fix, we have handled this scenrio. User
won't be able to upload garbage file. Incase of self signed cert upload
workflow, We are matching the modulus of
cert RSA Public key with the modulus of its private key if its matched
then it means certs are valid otheriwse Invalid.
For CA Cert, we are just validating the Cert format.
Added the unit Tests as well.

(cherry picked from commit 5f8d39a)

* Heading:
[5594][Platform] Handle invalid certs/keys correctly
Description:
We wanted to verify the cert at upload time itself.
Testing:
When we try to upload selfSigned cert, we were able to upload any
garbage files. Now with this fix, we have handled this scenrio. User
won't be able to upload garbage file. Incase of self signed cert upload
workflow, We are matching the modulus of
cert RSA Public key with the modulus of its private key if its matched
then it means certs are valid otheriwse Invalid.
For CA Cert, we are just validating the Cert format.
Added the unit Tests as well.

(cherry picked from commit 5f8d39a)

Co-authored-by: jitendra-12113 <jitedra.kumar@hashedin.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment