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(run): better handling of ghost linux user during ghost run #327

Closed
wants to merge 1 commit into from

Conversation

acburdine
Copy link
Member

@acburdine acburdine commented Jul 10, 2017

refs #311

  • add handling to linux extension that ensures the user is correct

@acburdine acburdine requested a review from sebgie July 10, 2017 09:57
@acburdine
Copy link
Member Author

also cc @ErisDS for the wording of the error messages

@acburdine acburdine requested a review from ErisDS July 10, 2017 09:58
@acburdine acburdine force-pushed the fix/run-permissions branch 2 times, most recently from d2fd651 to efc1df9 Compare July 10, 2017 10:10
Copy link
Contributor

@sebgie sebgie left a comment

Choose a reason for hiding this comment

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

Looks good, not tested on a server yet.

@acburdine
Copy link
Member Author

tested and verified that this works 👍

@ErisDS
Copy link
Member

ErisDS commented Jul 10, 2017

Meep. I have no context. What's going on here?

'Because you have set up a "ghost" system user, you must run ghost run via sudo.'

@ErisDS
Copy link
Member

ErisDS commented Jul 10, 2017

^ makes no sense to me because ghost cli sets up a "ghost" system user.

@acburdine
Copy link
Member Author

Sorry, more context. Because ghost-cli sets up the "ghost" system user, running ghost run won't work due to permissions of the content folder. This fixes that problem by setting the group and user of the process to that of the ghost user, but it requires ghost run to be initially started using sudo, hence the error message.

Updating it to be more clear.

@TryGhost TryGhost deleted a comment from coveralls Jul 10, 2017
@TryGhost TryGhost deleted a comment from coveralls Jul 10, 2017
@TryGhost TryGhost deleted a comment from coveralls Jul 10, 2017
@acburdine
Copy link
Member Author

@ErisDS wording is a bit better now (I think)

@ErisDS
Copy link
Member

ErisDS commented Jul 10, 2017

it requires ghost run to be initially started using sudo, hence the error message.

Does "initially" mean "the first time it is run"?

I'm also confused as to why this closes #307

@sebgie Please intervene here. I don't understand how what's happening in this PR relates to your permissions guide:

Please be careful here. ghost install is executed without sudo and whenever ghost-cli needs sudo privileges (e.g.: restart Nginx) it will ask for it. Using ghost-cli with sudo could lead to an inconsistent state since all operations would be executed in the context of the super user.

It is so hard right now to keep track of the context of why a decision was made. IMO this issue should be referencing a spec.

@acburdine
Copy link
Member Author

acburdine commented Jul 10, 2017

It closes #307 because this is kind of a two-fix PR - the first part (the one that closes #307) I will split out into a separate PR to not be as confusing.

First part = saving the content path to the config file instead of mutating process.env
Second part = sudo ghost run

This is related to both #303 and #311 - an actual write up of the spec for this change is in this comment

initially = every time, since ghost run is run in the foreground.

refs TryGhost#311
- add handling to linux extension that ensures the user is correct
@acburdine
Copy link
Member Author

The fix for #307 has been removed from this PR and split out into #329

@coveralls
Copy link

Coverage Status

Coverage remained the same at 56.231% when pulling 7d107d8 on acburdine:fix/run-permissions into a4da2a4 on TryGhost:master.

@TryGhost TryGhost deleted a comment from coveralls Jul 10, 2017
@sebgie
Copy link
Contributor

sebgie commented Jul 10, 2017

I agree with Hannah, given that all ghost-cli commands are run without sudo and ask for super user privileges when needed it feels very wrong to suddenly require sudo.

From Slack: https://ghost.slack.com/archives/C1QUCB9T9/p1499681674116302

sebastian [12:14] 
just for my understanding, why doesn’t ghost-cli ask for sudo?

austin [12:14] 
because it can't in-process

[12:15] 
it uses `process.setuid` and `process.setgid`

[12:15] 
which you can't use unless you're running as sudo originally

Is there a way to let the CLI ask for sudo if needed? I see that run.js uses child.spawn() and we could use sudo -u ghost node current/index.js in this call to use the ghost user?

@acburdine
Copy link
Member Author

We could, but that would require hardcoding of the Linux extension's behavior into Ghost-CLI itself. I'm willing to do that, but I think that implication does need to be considered.

@sebgie
Copy link
Contributor

sebgie commented Jul 10, 2017

We are going to make the ghost system user mandatory for Linux systems (#334). To resolve the ghost run discussion we are going to have a special runner for Linux.

@acburdine
Copy link
Member Author

Closing this as per #334

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