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

Added custom ca chain to deployments #599

Merged
merged 18 commits into from
Jul 25, 2024

Conversation

b1schumacher
Copy link
Contributor

@b1schumacher b1schumacher commented Jun 11, 2024

Description

Added the custom CA Chain variables to the values.yaml and added the ca to services and jobs.

Related Issue

Motivation and Context

Custom CA chains are required for multiple services, which require that ocis is trusting these ca's

How Has This Been Tested?

  • Tested the deployment with custom CA Chains

Screenshots (if appropriate):

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Technical debt
  • Tests only (no source changes)

Checklist:

@b1schumacher b1schumacher marked this pull request as ready for review June 12, 2024 08:26
@mmattel mmattel requested review from d7oc and wkloucek June 12, 2024 08:59
Comment on lines 83 to 86
{{- if .Values.customCAChain.enabled }}
- name: custom-ca-chain
mountPath: /etc/ssl/certs/
{{- end }}
Copy link
Contributor

@d7oc d7oc Jun 12, 2024

Choose a reason for hiding this comment

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

Would be good to have this fragment templated so we don't have these lines in all deployments.yml file but only one with a call here.

suggestion

{{/*
oCIS ca mount

@param .path          The path to mount
@param .scope.      The current scope
*/}}
{{- define "ocis.caPath" -}}
            {{- if .scope.Values.customCAChain.enabled }}
            - name: custom-ca-chain
              mountPath: {{ .path }}
            {{- end }}
{{- end -}}

Usage afterwards with {{- include "ocis.caPath" (dict "scope" . "path" /etc/ssl/certs) }}

@b1schumacher
Copy link
Contributor Author

I changed all the entries to a template and fixed also the issue that the customCAChain was overwriting the other CA files.

Now there are two different directories, one with the default files and one with the custom CA chain. Both are added to the SSL_CERT_DIR variable.

@wkloucek
Copy link
Contributor

@b1schumacher I applied some formatting and added enabled this feature in the linting process.

So far I didn't get the pattern for which services the custom CA has been configured / skipped. Could you maybe elaborate why it's not on all services?

Also what's missing are the CronJobs, some of them only talk to NATS but even for this, we could make use of this feature.

@b1schumacher
Copy link
Contributor Author

@wkloucek I added the missing services and the jobs.

@wkloucek wkloucek changed the base branch from main to stable-5 July 12, 2024 13:04
@wkloucek
Copy link
Contributor

I changed this to point to stable-5 instead of main which is now tracking ocis rolling releases

# -- Custom CA enables SSL_CERT_DIR in pods with the additional path /etc/ssl/custom.
enabled: false
# -- If custom CA chain is enabled this attribute mounts the existing secret to /etc/ssl/custom.
existingSecret: ""
Copy link
Contributor

@wkloucek wkloucek Jul 15, 2024

Choose a reason for hiding this comment

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

Is there a particular reason for the CA chain to be put in a secret? From what I know, a CA chain is public information and would therefore also be suitable for a ConfigMap.

Copy link
Contributor

Choose a reason for hiding this comment

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

I changed it to a ConfigMap in e344382

@wkloucek
Copy link
Contributor

@d7oc From my side this PR is ready for review / merge now.

You can give it a try by running the newly added deployment example. It'll create a CA and trust it on the oCIS side. It also will use the CA for generating a certificate for the Ingress and use this instead of the default dummy certificate (generated by most ingress controllers). Therefore we can actually skip enabling the insecure.oidcIdpInsecure and insecure.ocisHttpApiInsecure options and still login / upload files .

@wkloucek wkloucek requested a review from d7oc July 24, 2024 13:59
Copy link
Contributor

@d7oc d7oc left a comment

Choose a reason for hiding this comment

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

LGTM aside the typo. Also verified locally via k3d.

charts/ocis/values.yaml Outdated Show resolved Hide resolved
Co-authored-by: Dennis Sieben <[email protected]>
@wkloucek wkloucek requested a review from d7oc July 25, 2024 04:58
Copy link
Contributor

@d7oc d7oc left a comment

Choose a reason for hiding this comment

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

LGTM

@wkloucek wkloucek merged commit 12fb378 into owncloud:stable-5 Jul 25, 2024
1 check passed
@wkloucek wkloucek mentioned this pull request Jul 25, 2024
11 tasks
wkloucek added a commit to wkloucek/ocis-charts that referenced this pull request Jul 25, 2024
* add configuration for an additional ca chain

---------

Co-authored-by: schumacher <[email protected]>
Co-authored-by: Willy Kloucek <[email protected]>
Co-authored-by: Dennis Sieben <[email protected]>
@wkloucek wkloucek mentioned this pull request Jul 25, 2024
5 tasks
@wkloucek wkloucek mentioned this pull request Sep 9, 2024
butonic pushed a commit that referenced this pull request Oct 10, 2024
* add configuration for an additional ca chain

---------

Co-authored-by: schumacher <[email protected]>
Co-authored-by: Willy Kloucek <[email protected]>
Co-authored-by: Dennis Sieben <[email protected]>
@butonic butonic mentioned this pull request Oct 10, 2024
butonic pushed a commit that referenced this pull request Oct 29, 2024
* add configuration for an additional ca chain

---------

Co-authored-by: schumacher <[email protected]>
Co-authored-by: Willy Kloucek <[email protected]>
Co-authored-by: Dennis Sieben <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

support custom ca chain
4 participants