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

feat: Make cacert export save only CA certificates #859

Merged
merged 4 commits into from
Sep 21, 2020
Merged

feat: Make cacert export save only CA certificates #859

merged 4 commits into from
Sep 21, 2020

Conversation

mmorhun
Copy link
Contributor

@mmorhun mmorhun commented Sep 17, 2020 •
edited
Loading

Signed-off-by: Mykola Morhun mmorhun@redhat.com

What does this PR do?

Fixes cacert:export command to save only CA certificate from the certificate chain of the self-signed-certificate secret.

What issues does this PR fix or reference?

eclipse-che/che#16773

@mmorhun mmorhun requested a review from l0rd as a code owner September 17, 2020 13:33
@mmorhun mmorhun self-assigned this Sep 17, 2020
AndrienkoAleksandr
AndrienkoAleksandr reviewed Sep 17, 2020
View reviewed changes
src/api/che.ts Outdated Show resolved Hide resolved
@mmorhun mmorhun changed the title Make cacert:export save only the last certificate in the chain feat: Make cacert:export save only the last certificate in the chain Sep 17, 2020
@tolusha tolusha mentioned this pull request Sep 17, 2020
Closed
48 tasks
@mmorhun mmorhun changed the title feat: Make cacert:export save only the last certificate in the chain feat: Make cacert export save only CA certificates Sep 18, 2020
@mmorhun
Copy link
Contributor Author

mmorhun commented Sep 18, 2020

Rebased

tolusha
tolusha reviewed Sep 18, 2020
View reviewed changes
yarn.lock Outdated
@@ -294,7 +294,6 @@
"@kubernetes/client-node@0.11.2":
version "0.11.2"
resolved "https://registry.yarnpkg.com/@kubernetes/client-node/-/client-node-0.11.2.tgz#5dbc1d66d3c954af0f3bd294dece884737f5b761"
integrity sha512-Uhwd2y2qCvugICnHRC5h2MT5vw0a1dJPVVltVwmkeMuyGTPBccsTtpTcSfSLitwOrh4yr+9wG5bRcMdgeRjYPw==
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't know why but we have a lot of removed lines like this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Strange, I just use yarn as usual.... Will revert these changes

Copy link
Contributor Author

Choose a reason for hiding this comment

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

From what I see, yarn deletes them automatically.

Copy link
Contributor

Choose a reason for hiding this comment

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

integrity sha512-Uhwd2y2qCvugICnHRC5h2MT5vw0a1dJPVVltVwmkeMuyGTPBccsTtpTcSfSLitwOrh4yr+9wG5bRcMdgeRjYPw==

This thing generation depends on yarn version

@flacatus
Copy link
Collaborator

/test v5-chectl-e2e

@benoitf
Copy link
Collaborator

benoitf commented Sep 18, 2020

/retest

@benoitf
Copy link
Collaborator

benoitf commented Sep 19, 2020

hello, which version of yarn do you have ?
it's removing all integrity sha512-xxx lines in yarn.lock ?

tolusha
tolusha reviewed Sep 20, 2020
View reviewed changes
src/api/che.ts
for (const certPem of certsPem) {
const cert = nodeforge.pki.certificateFromPem(certPem)
const basicConstraintsExt = cert.getExtension('basicConstraints')
if (basicConstraintsExt && (basicConstraintsExt as any).cA) {
Copy link
Collaborator

@tolusha tolusha Sep 20, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

cA is it correct?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure. Otherwise it wouldn't work. (I also wondered why they named the field such way).

@mmorhun
Copy link
Contributor Author

mmorhun commented Sep 21, 2020

@benoitf I have 1.6.0 which I must update.

@mmorhun
Make cacert:export save only the last certificate in the chain
89748bc 
Signed-off-by: Mykola Morhun <mmorhun@redhat.com>
@mmorhun
Parse certs to get only CA ones
974bf5d 
Signed-off-by: Mykola Morhun <mmorhun@redhat.com>
@mmorhun
Fixes
a151f02 
Signed-off-by: Mykola Morhun <mmorhun@redhat.com>
@mmorhun
Update yarn.lock
20dae9a 
Signed-off-by: Mykola Morhun <mmorhun@redhat.com>
@openshift-ci-robot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: AndrienkoAleksandr, flacatus, mmorhun, tolusha
To complete the pull request process, please assign
You can assign the PR to them by writing /assign in a comment when ready.

The full list of commands accepted by this bot can be found 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

@mmorhun mmorhun merged commit 68646b8 into master Sep 21, 2020
@mmorhun mmorhun deleted the che-16773 branch September 21, 2020 09:23
@che-bot che-bot added this to the 7.19 milestone Sep 21, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tolusha tolusha tolusha approved these changes

@AndrienkoAleksandr AndrienkoAleksandr AndrienkoAleksandr approved these changes

@flacatus flacatus flacatus approved these changes

@l0rd l0rd Awaiting requested review from l0rd

@benoitf benoitf Awaiting requested review from benoitf

Assignees

@tolusha tolusha

@AndrienkoAleksandr AndrienkoAleksandr

@mmorhun mmorhun

@flacatus flacatus

Labels
lgtm
Projects
None yet
Milestone
7.19
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@mmorhun @flacatus @benoitf @openshift-ci-robot @tolusha @AndrienkoAleksandr @che-bot