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

fix: credential secret does not works if set from values #113

Closed
wants to merge 2 commits into from

Conversation

grhawk
Copy link

@grhawk grhawk commented Aug 28, 2022

Hi, thank you for your nice job!

I think I found a couple of minor issues. However, it could be I did not get the right way to use the chart :).

Double encoded secret

The main repo creates the connect-credentials secret using the stringData method but provides binary strings:

stringData:
{{ .Values.connect.credentialsKey }}: |-
{{- if (.Values.connect.credentials) }}
{{ .Values.connect.credentials | b64enc | indent 2 }}
{{- else }}
{{ .Values.connect.credentials_base64 | indent 2 }}
{{- end }}

This ends up encoding the string twice making the mounted file not accessible by the software. Using the data method works as expected since the helm chart takes care of converting to binary when needed.

1password-credential.json stored in an environmental variable

The main repo expects the server codes to use to read the credentials directly from the OP_SESSION variable. Thus the helm chart reads like this:

env:
- name: OP_SESSION
valueFrom:
secretKeyRef:
name: {{ .Values.connect.credentialsName }}
key: {{ .Values.connect.credentialsKey }}

This stores the value of the secret in the OP_SESSION variable but the docs of the op-server are quite clear on the fact that the variable only defines the path to the credential file.

I fixed this issue by creating a volume out of the credential-secret and mounting it always in the expected spot. I don't see the need of allowing the users of the chart to define directly the OP_SESSION. In case, this can be easily modified though.

@jgoret
Copy link

jgoret commented Sep 13, 2022

Thanks for this, that was exactly the problem I was getting!

Here's a kustomize patch that I used in my ArgoCD app to apply those changes, that could help others before the PR is merged

apiVersion: apps/v1
kind: Deployment
metadata:
  name: onepassword-connect
spec:
  template:
    spec:
      containers:
        - name: connect-api
          env:
            - name: OP_SESSION
              value: /home/opuser/.op/1password-credentials.json
              valueFrom:
          volumeMounts:
            - name: credentials
              mountPath: /home/opuser/.op/1password-credentials.json
              subPath: 1password-credentials.json
        - name: connect-sync
          env:
            - name: OP_SESSION
              value: /home/opuser/.op/1password-credentials.json
              valueFrom:
          volumeMounts:
            - name: credentials
              mountPath: /home/opuser/.op/1password-credentials.json
              subPath: 1password-credentials.json

@grhawk
Copy link
Author

grhawk commented Jun 15, 2023

@jillianwilson
Hi, do you think this PR is still relevant? :)

@ElanHasson
Copy link

@grhawk it's still a problem i guess-- I'm unable to use this chart.

Copy link
Contributor

⚠️ This PR contains unsigned commits. To get your PR merged, please sign those commits (git rebase --exec 'git commit -S --amend --no-edit -n' @{upstream}) and force push them to this branch (git push --force-with-lease).

If you're new to commit signing, there are different ways to set it up:

Sign commits with gpg

Follow the steps below to set up commit signing with gpg:

  1. Generate a GPG key
  2. Add the GPG key to your GitHub account
  3. Configure git to use your GPG key for commit signing
Sign commits with ssh-agent

Follow the steps below to set up commit signing with ssh-agent:

  1. Generate an SSH key and add it to ssh-agent
  2. Add the SSH key to your GitHub account
  3. Configure git to use your SSH key for commit signing
Sign commits with 1Password

You can also sign commits using 1Password, which lets you sign commits with biometrics without the signing key leaving the local 1Password process.

Learn how to use 1Password to sign your commits.

Watch the demo

@jillianwilson
Copy link
Contributor

Hey @grhawk thanks for the PR for this! I know this has been sitting here for a bit, but I reviewed today and tested and all looks good. My only feedback is that you sign your commit, as commits need to be signed to merge into main. I could sign your commit on your behalf but then it would look like I wrote your commit. I want to make sure you are properly credited for your work :)

@grhawk
Copy link
Author

grhawk commented May 21, 2024

Hi @jillianwilson, I ll sign the commit by the end of the week!
Thank you for your time!!

Best
Riccardo

@grhawk
Copy link
Author

grhawk commented May 25, 2024

I created a new PR since I could not sign the commit I did in this one (the rebase had conflicts and it was too long after I did the PR to remember what I did :))

I am closing this one and pls continue with #196

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.

4 participants