re: #2682 - Passing credentials to server command using a file, instead of the commandline #2691

Open
wants to merge 6 commits into
from

Projects

None yet

4 participants

@beniquezsd
Contributor
beniquezsd commented Jan 2, 2017 edited

Improvement for issue #2682: Cannot set blank username and password from shell script.

TL;DR:

With the current TiddlyWiki authentication scheme...

  • Having optional positional parameters in the middle of a command string can cause headaches when dealing with shells, parameter passing, and white space, as noted above.
  • On most shared hosting, anyone on the system can execute a simple ps -aux | grep tiddlywiki and get the the credentials for the wiki.

Solutions

  1. ....
  2. Replace the username and password fields with a "credentials-file" field, the contents of which can be left blank (or specified as /dev/null) to indicate a no access control.
  3. Use a Tiddler to store the authentication data.

Nothing too complicated, but decided to try and implement it with a tiddler file, so that it's consistent with the rest of the configuration options (like $:/config/tiddlyweb_host and others).

Please let me know if there are any coding style changes you'd like me to make. I did try and review the documentation, and mostly tried to emulate the style of the server.js file, but I wouldn't be surprised if there were small details I missed. :)

If this is accepted, it will probably require documentation changes as well... I'll try and look into how to do that.

@Jermolene
Owner

Thanks @beniquezsd I've added some comments over at #2682

@Jermolene
Owner

Hi @beniquezsd in terms of this PR, my preferred option would be to include the credentials information in the tiddlywiki.info file for the wiki. The content of the JSON file is available via $tw.boot.wikiInfo.

Ideally, we would add a new server property with individual settings for each of the parameters of the --server command. Any settings provided on the command line would override values provided in the tiddlywiki.info file.


	"server": {
		"port": "8080",
		"roottiddler": "$:/core/save/all",
		"rendertype": "text/plain",
		"servetype": "text/html",
		"username": "me",
		"password": "pass",
		"host": "example.com",
		"pathprefix": "wiki"
	}
@tobibeer
Contributor
tobibeer commented Jan 3, 2017

@Jermolene , Would it not be preferable to have users be an array with user objects in it?

@Jermolene
Owner

Would it not be preferable to have users be an array with user objects in it?

Well, there's nothing to stop us making that enhancement in the future, but right now the server.js only supports a single username and password, and this PR is specifically about how to configure that username and password.

@pmario
Contributor
pmario commented Jan 3, 2017

hmmm,
username and password in plain text and pushed to github? .... nightmare!

Imo it should be environment variables at least.

@beniquezsd
Contributor
beniquezsd commented Jan 7, 2017 edited

@pmario I don't understand your concern... an I understand it, the $:/config/tiddler and the tiddlywiki.info files are instance configuration files... the only thing that would be pushed to Github would be the "default files", which would just be blank.

@Jermolene That change would involve modifying boot/boot.js and modifying the wiki "editions" used by core/modules/command/init.js, and the server.js command, correct? Am I missing anything?

@Jermolene
Owner

Hi @beniquezsd I think @pmario's concern is that we might be making it too easy for people to commit credentials to Git(Hub). It seems to be more conventional to use environment variables to pass credentials around.

However, I think that concern only applies to the password, so the question might be whether we support the new mechanism for that specific parameter.

Anyhow, I will leave a couple of code review comments. Also please could you put the CLA signature in a separate PR?

@beniquezsd
Contributor
beniquezsd commented Jan 9, 2017 edited

Forgive me if I sound ignorant, but do a lot of TiddlWiki users check in their wiki instances to github? Why would they care about locking their TiddlyWiki with a username and password if they check in the content to github?

If there's a big danger in people checking in their password to github, there's always hashed passwords

Regarding the password field, I don't think removing it from the TiddlyWiki.info file would achieve much, either. It would add inconsistency, and worse, force the users that do want to set a password to specify the other positional parameters that come before password, on the commandline, due to the way parameters are handled currently.

Personally, I dont think TiddlyWiki needs Wikleaks-level security... I just wanted to be able to launch it from a script without a password, while being able to specify the listening interface and a URL subdir.

For me it was about configuration flexibility. The added benefit of moving the credentials away from the command line was just a bonus for me. :)

I'll sign the CLA on github as well as the PR in a few moments.

@beniquezsd beniquezsd added a commit to beniquezsd/TiddlyWiki5 that referenced this pull request Jan 9, 2017
@beniquezsd beniquezsd Update cla-individual.md
For [PR69](Jermolene#2691)
b155178
@pmario
Contributor
pmario commented Jan 9, 2017

@beniquezsd

Personally, I dont think TiddlyWiki needs Wikleaks-level security...

.... hmmmm, me too, but consider these!

  1. This pull request has its origin in #2682 and a quirks, that may come from bash parameter handling of your hosting provider. So for me in the first instance it should be resolved there, because what we are doing here is "fighting a symptom" not really solving the initial problem!

.... but ....

  1. bash parameter handling is indeed tricky, especially if you create nested scripts, which seems to be the case in #2682. ...

  2. An other problem comes up, if we have to create empty parameters for the windows powershell, where single quotes and double quotes "really" have a different meaning. So getting a generic solution for windows is a plus for me.

  3. Most importantly. ... What ever we put into the core, we can't take it back in an easy way. ... So if we implement a "carelessly weak mechanism", we have to live with it for ever!

@pmario
Contributor
pmario commented Jan 9, 2017

@Jermolene

However, I think that concern only applies to the password, so the question might be whether we support the new mechanism for that specific parameter.

The concern also applies to the user name. eg: if it is. anyName@example.com it already contains more information, that I personally don't want to share that way. ... Users are lazy and may use the same PW with different services. So having the username in plaintext should be avoided too!!

@pmario
Contributor
pmario commented Jan 9, 2017

@beniquezsd

Forgive me if I sound ignorant, but do a lot of TiddlWiki users check in their wiki instances to github? Why would they care about locking their TiddlyWiki with a username and password if they check in the content to github?

It doesn't matter, if it makes sense to push sensitive data to github. ... If it's possible, it will happen. That's the real problem!

@beniquezsd
Contributor

@pmario For what it's worth, I don't think I was able to properly articulate my problem. Although I admit in my original issue report that its an edge case.

It's not so much about the single quotes, or double-quotes, but about the way Bash interprets whitespace by default. When passing blank parameters to a secondary script, the secondary script doesnt interpret "" or '' as an empty string variable. It "eats" the parameters.

As I see it, telling users to change shells isn't a good solution. I don't currently have the privilege to control my host, and Bash is the most widely used UNIX shell...

Personally, I prefer to configure such things on the filesystem, where I can lock up the files with the proper permissions, at my own hidden location.

@Jermolene proposed a solution that works for me, and in addition adds flexibility to the server configuration. I deferred to his judgement, and the solution works for me.

This PR doesn't take anything away from the existing parameter handling; it adds a new data source for those variables. Your powershell scripts will still function normally.


Where I WOULD disagree with you is with the following statement:

Most importantly. ... What ever we put into the core, we can't take it back in an easy way. ... So if we implement a "carelessly weak mechanism", we have to live with it for ever!

I would argue that the current method can be considered " carelessly weak" as it is... but I'm not here to change that, really. I don't presume to know much about TiddlyWiki's architecture...

I tried to keep this as a minor code change, if possible. Nothing is being changed significantly in core (besides some default values , which now come from a .info file).

I'm also skeptical of the claim that core will remain static for 500 years, let alone "forever".
Please let me know if there's an established "code freeze" or some protocol I'm not aware of; I admit that I could be out of line here, since I'm out of the loop. :P

@tobibeer
Contributor
tobibeer commented Jan 9, 2017

Not having a better idea for how to implement this via registry or such, I think of all concerns raised so far, the one where user credentials and passwords are stored intermingled with critical configuration is the one that I would agree most that it's not representing a desirable strategy.

@Jermolene Jermolene added a commit that referenced this pull request Jan 10, 2017
@beniquezsd @Jermolene beniquezsd + Update cla-individual.md (#2704)
For [PR69](#2691)
c3833d0
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment