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

Configure SystemRead on weekly.ci #3993

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

NotMyFault
Copy link
Member

Is this safe to enable? Given weekly.ci is a show-off instance, with a public configuration, I'd assume there's nothing sensitive extended read would expose, would it?

@NotMyFault NotMyFault requested a review from a team May 25, 2023 15:38
@dduportal
Copy link
Contributor

Is this safe to enable? Given weekly.ci is a show-off instance, with a public configuration, I'd assume there's nothing sensitive extended read would expose, would it?

Credentials at least (I guess there is a GH App credential at least).

Ping @daniel-beck @Wadeck @timja @MarkEWaite @jglick can i still your knowledge to help us answer Alex's question here?

As an OPS, I have a bit of reluctance but maybe that it me being too paranoid.

Additionnaly: setting the whole system in full tmpfs (instead of persistence) with a daily reset could leverage risks (and avoid any future manual changes in the UI without putting it as code)

@jglick
Copy link

jglick commented May 25, 2023

I am not able to see weekly.ci, whatever that is, so I can offer no advice.

@timja
Copy link
Member

timja commented May 25, 2023

The point of system read is that nothing sensitive should be exposed assuming plugins are being sensible I doubt there’s anything that would be bad but would need to check it (it’s down atm)

@dduportal
Copy link
Contributor

The point of system read is that nothing sensitive should be exposed assuming plugins are being sensible I doubt there’s anything that would be bad but would need to check it (it’s down atm)

https://weekly.ci.jenkins.io/ is down for you? (I can see it)

@timja
Copy link
Member

timja commented May 26, 2023

The point of system read is that nothing sensitive should be exposed assuming plugins are being sensible I doubt there’s anything that would be bad but would need to check it (it’s down atm)

weekly.ci.jenkins.io is down for you? (I can see it)

You can see it now, it was down at the time I checked

@dduportal
Copy link
Contributor

Thanks for your message @timja : it allowed us to see that weekly.ci.jenkins.io was restarting regularly due to its helm release failing to deploy changes in #3991 (rollbacked in #3996). It should be stable now: can you confirm?

(cc @NotMyFault )

@timja
Copy link
Member

timja commented May 26, 2023

I checked https://weekly.ci.jenkins.io/manage/configuration-as-code/

the only two interesting values I saw are the ldap configuration and the github app for weekly.

It looks fine to me but up to you all.

@dduportal
Copy link
Contributor

I checked https://weekly.ci.jenkins.io/manage/configuration-as-code/

the only two interesting values I saw are the ldap configuration and the github app for weekly.

It looks fine to me but up to you all.

The LDAP password should not be available at all (even encrypted) in an ideal world.
Is this mandatory to use the LDAP for authentication on this instance (vs. the default security realm with password encrypted in the helm chart)?

For the GH app, it's up to you @timja as I believe it's only for jenkinsci GH organization: is that correct?

@timja
Copy link
Member

timja commented May 29, 2023

Is this mandatory to use the LDAP for authentication on this instance (vs. the default security realm with password encrypted in the helm chart)?

I think it's fine without ldap

For the GH app, it's up to you @timja as I believe it's only for jenkinsci GH organization: is that correct?

it is yes, tbh not sure if it even needs a github app, but fine for it as-is

@dduportal
Copy link
Contributor

Is this mandatory to use the LDAP for authentication on this instance (vs. the default security realm with password encrypted in the helm chart)?

I think it's fine without ldap

For the GH app, it's up to you @timja as I believe it's only for jenkinsci GH organization: is that correct?

it is yes, tbh not sure if it even needs a github app, but fine for it as-is

Thanks for confirming. I propose (cc @NotMyFault ) that we update weekly.ci.jenkins.io setup to ensure we can use the System read safely:

  • Switch from LDAP to "local jenkins user database"
  • Remove the usage of GitHub app credential

@NotMyFault
Copy link
Member Author

Is this mandatory to use the LDAP for authentication on this instance (vs. the default security realm with password encrypted in the helm chart)?

I think it's fine without ldap

For the GH app, it's up to you @timja as I believe it's only for jenkinsci GH organization: is that correct?

it is yes, tbh not sure if it even needs a github app, but fine for it as-is

Thanks for confirming. I propose (cc @NotMyFault ) that we update weekly.ci.jenkins.io setup to ensure we can use the System read safely:

  • Switch from LDAP to "local jenkins user database"
  • Remove the usage of GitHub app credential

Works for me 👍

@Wadeck
Copy link

Wadeck commented May 30, 2023

From his message, I assume that Jesse was not aware of https://weekly.ci.jenkins.io/, as it was refered to "weekly.ci" only.

From what I can see the https://weekly.ci.jenkins.io/manage/credentials/store/system/domain/_/credential/github-app-weekly/ credentials could be removed as it's used only for

credentialsId: "github-app-weekly"
remote: "https://github.com/jenkins-infra/pipeline-library.git"
, which is a public repository. I don't want to receive security reports on such stuff asking for a bounty :)

(💡 same thing with

credentialsId: "github-app-infra"
remote: "https://github.com/jenkins-infra/pipeline-library.git"
for the infra instance)

💡 I don't know if the configuration of "pipeline-library" is for the demo purpose, it's not used. None of the pipeline is using shared libraries.

No concerns from my side 👍

@daniel-beck
Copy link
Contributor

credentials could be removed

Rate limiting can be an issue, but no real danger in app credentials that can read a single repo (or even all public repos in the org), which is how they should be configured.

@Wadeck
Copy link

Wadeck commented May 30, 2023

Rate limiting can be an issue

The shared library is not used so 🤷‍♂️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants