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

Make systems CAs platform-independent using injectables #6321

Merged
merged 9 commits into from
Nov 22, 2022
Merged

Make systems CAs platform-independent using injectables #6321

merged 9 commits into from
Nov 22, 2022

Conversation

Nokel81
Copy link
Collaborator

@Nokel81 Nokel81 commented Oct 4, 2022 •
edited
Loading

  • Make injecting CAs injectable, remove mac-ca as dependency
  • Fix win-ca failing on electron renderer on windows
  • Fix the matcher under features/ for main

Signed-off-by: Sebastian Malton sebastian@malton.name

This is a reintroduction of the PR which I will work on to see why it causes windows integration tests to fail.

@Nokel81 Nokel81 added this to the 6.2.0 milestone Oct 4, 2022
@Nokel81 Nokel81 requested a review from a team as a code owner October 4, 2022 21:58
@Nokel81 Nokel81 requested review from Iku-turso and ixrock and removed request for a team October 4, 2022 21:58
@github-actions
Copy link
Contributor

github-actions bot commented Oct 5, 2022

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@github-actions
Copy link
Contributor

Conflicts have been resolved. A maintainer will review the pull request shortly.

@github-actions
Copy link
Contributor

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@github-actions
Copy link
Contributor

Conflicts have been resolved. A maintainer will review the pull request shortly.

@github-actions
Copy link
Contributor

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@github-actions
Copy link
Contributor

Conflicts have been resolved. A maintainer will review the pull request shortly.

@github-actions
Copy link
Contributor

github-actions bot commented Nov 2, 2022

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@github-actions
Copy link
Contributor

github-actions bot commented Nov 2, 2022

Conflicts have been resolved. A maintainer will review the pull request shortly.

@github-actions
Copy link
Contributor

github-actions bot commented Nov 3, 2022

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@github-actions
Copy link
Contributor

github-actions bot commented Nov 3, 2022

Conflicts have been resolved. A maintainer will review the pull request shortly.

@github-actions
Copy link
Contributor

github-actions bot commented Nov 8, 2022

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@github-actions
Copy link
Contributor

github-actions bot commented Nov 8, 2022

Conflicts have been resolved. A maintainer will review the pull request shortly.

@Nokel81 Nokel81 modified the milestones: 6.2.0, 6.3.0 Nov 16, 2022
@github-actions
Copy link
Contributor

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@Nokel81
Remove mac-ca usage since it was only in tests (#6043)
7a37e2a 
* Make injecting CAs injectable, remove mac-ca as dependency
* Fix win-ca failing on electron renderer on windows
* Fix the matcher under features/ for main

Signed-off-by: Sebastian Malton <sebastian@malton.name>
@Nokel81
Fix type errors from new types
048bbe8 
Signed-off-by: Sebastian Malton <sebastian@malton.name>
@Nokel81
Temp change to see windows errors on CI
4fa122d 
Signed-off-by: Sebastian Malton <sebastian@malton.name>
@Nokel81
Fix temp change
5bc6c10 
Signed-off-by: Sebastian Malton <sebastian@malton.name>
@Nokel81
Change error message for windows
5019781 
Signed-off-by: Sebastian Malton <sebastian@malton.name>
@Nokel81
Increase maxBuffer size when reading windows CAs
18b6b4c 
Signed-off-by: Sebastian Malton <sebastian@malton.name>
@Nokel81
Switch back to running integration tests on windows
012ee23 
Signed-off-by: Sebastian Malton <sebastian@malton.name>
@Nokel81
Fix usage after rebase
0f884a4 
Signed-off-by: Sebastian Malton <sebastian@malton.name>
@Nokel81
Update lock file
71eed3a 
Signed-off-by: Sebastian Malton <sebastian@malton.name>
@github-actions
Copy link
Contributor

Conflicts have been resolved. A maintainer will review the pull request shortly.

jim-docker
jim-docker approved these changes Nov 22, 2022
View reviewed changes
Copy link
Contributor

@jim-docker jim-docker left a comment

Choose a reason for hiding this comment

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

Seems the title should be more like "Make systems CAs platform-independent using injectables"

There are a few ride-alongs here

src/common/certificate-authorities/inject-system-cas.injectable.ts
if (Array.isArray(globalAgent.options.ca) && !globalAgent.options.ca.includes(cert)) {
globalAgent.options.ca.push(cert);
} else {
globalAgent.options.ca = [cert];
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks right for when globalAgent.options.ca is not an array but it will also overwrite an existing array that already has cert plus possibly more certs. Is this OK?

I see this is old code. Maybe I made this comment before? :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That is a good point, I will make a follow up PR to fix this.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

#6628

src/common/fs/exec-file.injectable.ts
resolve({
callWasSuccessful: true,
response: stdout,
instantiate: (): ExecFile => {
Copy link
Contributor

Choose a reason for hiding this comment

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

not sure why this needs to be refactored here

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Because there are some cases in this PR where I need to pass options but not args

Copy link
Contributor

Choose a reason for hiding this comment

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

I looked for that, didn't see it. Oh, guess it is for the win32 case

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes

@Nokel81 Nokel81 merged commit 0eee5a0 into master Nov 22, 2022
@Nokel81 Nokel81 deleted the reintroduce-remove-mac-ca branch November 22, 2022 20:23
@Nokel81 Nokel81 changed the title Remove mac-ca usage since it was only in tests (#6043) Make systems CAs platform-independent using injectables Nov 22, 2022
@Nokel81 Nokel81 mentioned this pull request Dec 20, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jim-docker jim-docker jim-docker approved these changes

@jansav jansav jansav left review comments

@Iku-turso Iku-turso Awaiting requested review from Iku-turso Iku-turso is a code owner automatically assigned from lensapp/lens-ide

@ixrock ixrock Awaiting requested review from ixrock ixrock is a code owner automatically assigned from lensapp/lens-ide

Assignees
No one assigned
Labels
area/security chore
Projects
None yet
Milestone
6.3.0
Development

Successfully merging this pull request may close these issues.
3 participants
@Nokel81 @jansav @jim-docker