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

[Question][GitHub]security #2800

Closed
tk103331 opened this issue Aug 23, 2022 · 9 comments
Closed

[Question][GitHub]security #2800

tk103331 opened this issue Aug 23, 2022 · 9 comments
Assignees
Labels
type/question This issue is a question

Comments

@tk103331
Copy link
Contributor

Question

I found in the data returned by the '/api/pipelines/{id}/tasks' api that the PAT of github is displayed in plain text, whether this is a security issue.
I think this PAT should only be used when the backend performs tasks, which means that the frontend should not get the plaintext result.

Screenshots

image

Additional context

Add any other context here.

@tk103331 tk103331 added the type/question This issue is a question label Aug 23, 2022
@tk103331 tk103331 changed the title [Question][Module Name] Question title [Question][]security Aug 23, 2022
@tk103331 tk103331 changed the title [Question][]security [Question][GitHub]security Aug 23, 2022
@mindlesscloud
Copy link
Contributor

Thanks for your feedback, it will be fixed in the upcoming version. By the way, your PAT appears in two places, and you only covered one, please make sure to revoke it.

@mindlesscloud
Copy link
Contributor

@tk103331 After discussion with the team, we decided not to implement this feature before the config-ui supports authentication. Due to the lack of authentication, Devlake should be deployed in a secure environment.

@tk103331
Copy link
Contributor Author

According to your team's plan. But I think it is necessary!
PAT is hidden in the configuration page of the GitHub plugin.

image

@klesh
Copy link
Contributor

klesh commented Aug 26, 2022

Yes, it is hidden from UI, but visible at the API level:
image
It is necessary for API to return those values in order to support Editing.

In terms of security:

I suggest that you deploy devlake/config-ui inside Intranet only, avoiding exposing them to the Internet.
You may set Environment Variables ADMIN_USER/ADMIN_PASS for config-ui to enable the Basic Authentication.

@tk103331
Copy link
Contributor Author

Yes, I will inside Intranet only
I think the PAT (or other password) can only appears once in clear text in the frontend UI, that is when the user types it. And the backend should be encrypted and stored (the encryption and decryption algorithm can be determined by the plugin).

These are just my thoughts and suggestions. Maybe it is over-designed.

@klesh klesh reopened this Aug 29, 2022
@klesh
Copy link
Contributor

klesh commented Aug 29, 2022

@tk103331 "Appears only once" is not very practical, because some validations happen in Frontend, and it is ok for webpage to display sensitive information, take 1password as an example, we can copy password from its webapp without problem as long as https is enabled, you may do the same for config-ui in case you have security concerns.
But, I think there is a point in encrypting data in database, actually, we encrypt connection tokens and password in the database, but not the plan which contains the git repo secret string.
I think we should encrypt the pipeline.plan and buleprint.plan and blueprint.settings in the database if sb gains access to the database without config-ui, is that what you suggested?

@tk103331
Copy link
Contributor Author

I'm still new to the devlake project and am learning some details of this project.

At first, I found plaintext PAT in the API return result, and later found that the plaintext is stored in the _devlake_blueprints.plan field. But in _tool_github_connections.token is encrypted.

I have some experience in CI Pipeline, when a task in the pipeline requires sensitive data, it will only be decrypted and used when the task is executed.So I recommend decrypting sensitive data only while the SubTask is executing.

This shouldn't be a very important issue, and hopefully it won't affect your plans. 😃

@klesh
Copy link
Contributor

klesh commented Aug 29, 2022

@tk103331 Nah, it is important. I agree with you totally. I'm so glad that you join us.
So, here is my sum up:

  1. config-ui would be able to see the PlainText for those who have access to it (with optional Authentication and HTTPS)
  2. database would NOT store the PlainText but Encrypted Data, in case of Database Leaks

I will create an issue to track the progress of development if we are on the same page.

Thanks very much for the suggestion!

@klesh
Copy link
Contributor

klesh commented Aug 29, 2022

To be addressed in #2868. Close the question, and feel free to re-open if needed.
Thanks for the input @tk103331

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type/question This issue is a question
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants