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: restrict secretFile path #669

Closed

Conversation

MichaelDeSteven
Copy link
Contributor

What this PR does:

Which issue(s) this PR fixes:

Fixes #631

Special notes for your reviewer:

Does this PR introduce a user-facing change?:


cmd/layotto/main.go Outdated Show resolved Hide resolved
wrap the existing component to restrict secretFile path
components/secret/local_file_wrapper.go Outdated Show resolved Hide resolved
pkg/runtime/options.go Outdated Show resolved Hide resolved
pkg/runtime/secretstores/local/wrapper.go Outdated Show resolved Hide resolved
@mosn-community-bot mosn-community-bot bot added size/M and removed size/L labels Jul 5, 2022
func getPrefixConfigFilePath() string {
prefix := ""
for i, str := range os.Args {
if str == "-c" || str == "-config" {
Copy link
Member

Choose a reason for hiding this comment

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

The FIXME said that we should get the startup parameters in main.go and you can take

stm := stagemanager.InitStageManager(c, c.String("config"), app)
as an example.
Checking -c or -config here is not enough, since users can just ./layotto start and layotto will use the default configuration path

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The FIXME said that we should get the startup parameters in main.go and you can take

stm := stagemanager.InitStageManager(c, c.String("config"), app)

as an example.
Checking -c or -config here is not enough, since users can just ./layotto start and layotto will use the default configuration path

Is there means I can get configuration path by used c.String("config") in main.go to replace if str == "-c" || str == "-config"?

Copy link
Member

Choose a reason for hiding this comment

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

Yes! you can get the path by c.String("config") in main.go and then put it in a common place in the memory

We need to review the code and find a suitable place to store the path

Copy link
Contributor Author

@MichaelDeSteven MichaelDeSteven Jul 8, 2022

Choose a reason for hiding this comment

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

how to store var in memory? Can you give me some refs?

Copy link
Member

Choose a reason for hiding this comment

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

The easiest way is to store it in the main.go as a global variable. But this "global variable" way is not "elegant".
I suggest you read the startup code and try to pass the var to the runtime server without using global variable.
If you really can't find a way, then use global variable

Copy link
Member

Choose a reason for hiding this comment

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

@MichaelDeSteven Hi ,are u still working on it?
Do you need help? I can help with this

Copy link
Contributor Author

@MichaelDeSteven MichaelDeSteven Jul 16, 2022

Choose a reason for hiding this comment

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

sorry that I cant find the method of store var in startup code. can you give me some tips?

Copy link
Member

Choose a reason for hiding this comment

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

Sure. let me take a look

@seeflood
Copy link
Member

seeflood commented Jul 6, 2022

I submitted #698 to test the case ./layotto start -c and ./layotto start --config

@MichaelDeSteven Could u modify the quickstart doc to test the case 2 in #631

@MichaelDeSteven
Copy link
Contributor Author

I submitted #698 to test the case ./layotto start -c and ./layotto start --config

@MichaelDeSteven Could u modify the quickstart doc to test the case 2 in #631

I will modify and test it later.

@seeflood
Copy link
Member

seeflood commented Jul 7, 2022

Hi @MichaelDeSteven , I submitted a PR MichaelDeSteven#3 to your branch.
It will test cases including:

Please review and merge :)

@github-actions
Copy link

This pull request has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

@github-actions github-actions bot added the stale label Aug 16, 2022
@github-actions
Copy link

This pull request has been automatically closed because it has not had activity in the last 37 days. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

@github-actions github-actions bot closed this Aug 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working cla:yes size/M stale
Projects
None yet
Development

Successfully merging this pull request may close these issues.

the relative path in secretFile is error-prone; 限制 secretFile 能访问的路径
4 participants