TiddlyWiki5 icon indicating copy to clipboard operation
TiddlyWiki5 copied to clipboard

mws authentication

Open webplusai opened this issue 1 year ago • 34 comments

webplusai avatar Sep 12 '24 18:09 webplusai

Review PR in StackBlitz Codeflow Run & review this pull request in StackBlitz Codeflow.

Confirmed: webplusai has already signed the Contributor License Agreement (see contributing.md)

github-actions[bot] avatar Sep 12 '24 18:09 github-actions[bot]

Thank you @webplusai this is looking good. I will provide some detailed feedback shortly.

Jermolene avatar Sep 18 '24 11:09 Jermolene

Following along (very busy at work atm, company is reorging... again). This is exciting stuff!

joshuafontany avatar Sep 23 '24 19:09 joshuafontany

@Jermolene I've fixed the test issue. Also some issues that I had related to the user list and roles were also fixed.

webplusai avatar Oct 01 '24 07:10 webplusai

Thank you @webplusai. As discussed I am keen to get this merged. There are some points I would like to return to but we can worry about those later.

I think the blocker now is that we've got users who are testing the multi-wiki-support branch, and it is going to be a bad experience for them if they upgrade and then are presented with a login dialogue, with no obvious way to create or access a user account.

Ideally, we should get to the point where by default the user gets the previous experience of anonymous read/write access, with an obvious route to get to an admin screen so that they can enable password access and try things out.

One possibility would be to add commands like --mws-add-user, --mws-list-users etc. and use them to set up a demo configuration in a new npm command.

Let me know if that makes sense, ideally we should discuss the details before spending too much time on implementation.

Jermolene avatar Oct 01 '24 10:10 Jermolene

@Jermolene I've added the commands as you indicated.

webplusai avatar Oct 02 '24 09:10 webplusai

@webplusai ... Is there some description which mechanisms are used with the whole authentication system?

@Jermolene ... As I wrote sensible data should not be part of the same database as the content. They should be completely separated. IMO this will also help to use other systems that provide secure vaults in the future.

pmario avatar Oct 02 '24 09:10 pmario

  • I did run: npm run mws-add-user
  • Then I did run npm start

Is this order right, or did I do it wrong?

Which does start the server and I do get the user UI. But if I try to create a new bag, I do get a server-runtime error

grafik

syncer-server-filesystem: Dispatching 'save' task: $:/StoryList
Serving on http://127.0.0.1:8080
(press ctrl-C to exit)
$:/plugins/tiddlywiki/multiwikiserver/modules/routes/helpers/acl-middleware.js:31
    var partiallyDecoded = entityName.replace(/%3A/g, ":");
                                      ^

TypeError: Cannot read properties of undefined (reading 'replace')
    at exports.middleware ($:/plugins/tiddlywiki/multiwikiserver/modules/routes/helpers/acl-middleware.js:31:39)
    at exports.handler ($:/plugins/tiddlywiki/multiwikiserver/routes/handlers/post-bag.js:31:2)
    at IncomingMessage.<anonymous> ($:/plugins/tiddlywiki/multiwikiserver/mws-server.js:525:10)
    at IncomingMessage.emit (node:events:519:28)
    at endReadableNT (node:internal/streams/readable:1696:12)
    at process.processTicksAndRejections (node:internal/process/task_queues:82:21)

Node.js v20.14.0
PS E:\git\tiddly\tiddlywiki\TiddlyWiki5>

pmario avatar Oct 02 '24 11:10 pmario

Thanks @webplusai

Great to have your feedback @pmario, this is a great stage to be hammering out some important details.

Jermolene avatar Oct 02 '24 11:10 Jermolene

  • I did run: npm run mws-add-user
  • Then I did run npm start

Is this order right, or did I do it wrong?

Which does start the server and I do get the user UI. But if I try to create a new bag, I do get a server-runtime error

grafik

syncer-server-filesystem: Dispatching 'save' task: $:/StoryList
Serving on http://127.0.0.1:8080
(press ctrl-C to exit)
$:/plugins/tiddlywiki/multiwikiserver/modules/routes/helpers/acl-middleware.js:31
    var partiallyDecoded = entityName.replace(/%3A/g, ":");
                                      ^

TypeError: Cannot read properties of undefined (reading 'replace')
    at exports.middleware ($:/plugins/tiddlywiki/multiwikiserver/modules/routes/helpers/acl-middleware.js:31:39)
    at exports.handler ($:/plugins/tiddlywiki/multiwikiserver/routes/handlers/post-bag.js:31:2)
    at IncomingMessage.<anonymous> ($:/plugins/tiddlywiki/multiwikiserver/mws-server.js:525:10)
    at IncomingMessage.emit (node:events:519:28)
    at endReadableNT (node:internal/streams/readable:1696:12)
    at process.processTicksAndRejections (node:internal/process/task_queues:82:21)

Node.js v20.14.0
PS E:\git\tiddly\tiddlywiki\TiddlyWiki5>

@pmario you did it correct. There was an error and I fixed it.

webplusai avatar Oct 04 '24 08:10 webplusai

@Jermolene @pmario All of the major comments are fixed except the ones that @Jermolene mentioned to do later.

webplusai avatar Oct 04 '24 08:10 webplusai

Hi @pmario I've used tabs for indent.

webplusai avatar Oct 08 '24 07:10 webplusai

Hi @webplusai I checked out the latest commits, ran npm install and then npm test, but received the following error:

Running Server Test: Check index page
Failed "Check index page" with "Test failed"
Running Server Test: Upload a 1px PNG
$:/plugins/tiddlywiki/multiwikiserver/commands/mws-test-server.js:137
                        return jsonData["imported-tiddlers"] && $tw.utils.isArray(jsonData["imported-tiddlers"]) && jsonData["imported-tiddlers"][0] === "One White Pixel";
                                       ^

TypeError: Cannot read properties of undefined (reading 'imported-tiddlers')
    at Object.expectedResult ($:/plugins/tiddlywiki/multiwikiserver/commands/mws-test-server.js:137:19)
    at IncomingMessage.<anonymous> ($:/plugins/tiddlywiki/multiwikiserver/commands/mws-test-server.js:98:33)
    at IncomingMessage.emit (node:events:529:35)
    at endReadableNT (node:internal/streams/readable:1400:12)
    at process.processTicksAndRejections (node:internal/process/task_queues:82:21)

Node.js v18.20.3

Jermolene avatar Oct 08 '24 14:10 Jermolene

@Jermolene I've fixed all of your comments.

webplusai avatar Oct 10 '24 09:10 webplusai

Hi @webplusai that's great. I want to merge this now because it will make further refinements easier to be working with smaller PRs. But right now the user experience is broken. If somebody downloads the branch and runs npm start they get a login prompt that they will never be able to log into. Adding --build mws-add-user makes it easier to get up and running, but at the unacceptable cost of embedding an extremely dangerous security practice.

What are the options? I think the ideal user experience would be for a fresh install of MWS to have anonymous access, requiring configuration to enable access control. They should be able to do that configuration in the web UI or on the command line.

I do think the two things we need to pay attention to now are:

  • The user experience for people who want to try out MWS. As discussed, I think the plugin readme is the best place to focus the docs so that we keep them together. I can subsequently update the description of the main PR with up-to-date instructions
  • Documentation including commands, endpoints and an overview of the security model

Jermolene avatar Oct 14 '24 10:10 Jermolene

@Jermolene we can add a flag to the start command that instructs the server to allow anonymous users or not. What do you think?

webplusai avatar Oct 14 '24 18:10 webplusai

@Jermolene we can add a flag to the start command that instructs the server to allow anonymous users or not. What do you think?

Flags on the start command are not easily discoverable.

I think the ideal situation is that out of the box the database defaults to allowing anonymous read/write access to all resources. Users can then enable access control either via the web interface or via equivalent --mws-* commands.

We obviously cannot have the situation where a database configured to have access control can be easily downgraded to anonymous access.

If it's not clear, the key distinction for command line activities is that operations that permanently modify the database should be --mws-* subcommands, while options that affect how the --mws-listen command operates should be name=value arguments to the listen command. So, there might well be a sequence of --mws-* commands that would strip the access control from a database, but there should never be a listen option that bypasses access control.

Jermolene avatar Oct 15 '24 09:10 Jermolene

I think having anonymous access by default is a security risk. I would prefer "secure by default", even if it is different at the moment.

I would be OK with a --mws-dangerous-mode switch to allow anonymous r/w access

pmario avatar Oct 15 '24 10:10 pmario

I think having anonymous access by default is a security risk. I would prefer "secure by default", even if it is different at the moment.

I don't think that's a practical goal. I think it's better to match the existing behaviour of TW under Node.js.

How would it work to be "secure by default"? Think it through: a user downloads the repo and does npm install && npm start. How can that be secure by default? The best I can think of is an interactive wizard that kicks in at startup to ask the user how they want their server configured. But that is precisely equivalent from a user perspective to starting with an anon installation and then visiting the web UI to configure it how they want. But making it be a command line wizard is an awful user experience for most users.

It's easy to repeat catchphrases like "secure by default" but I don't think it's very useful without some critical analysis.

I would be OK with a --mws-dangerous-mode switch to allow anonymous r/w access

That is a hopeless, user hostile pattern that I would never consider for the core.

If you are really so worried about anonymous access by default you need to be a bit more detailed about the threat model you are concerned with.

Jermolene avatar Oct 15 '24 10:10 Jermolene

...But that is precisely equivalent from a user perspective to starting with an anon installation and then visiting the web UI to configure it how they want. But making it be a command line wizard is an awful user experience for most users.

I am OK, if the first (non skippable) step of the standard anonymous web-UI is setting up an admin account, and the dashboard of the admin prominently shows the "Add user account" page as long as there is no user other than admin.

pmario avatar Oct 15 '24 10:10 pmario

I am OK, if the first (non skippable) step of the standard anonymous web-UI is setting up an admin account, and the dashboard of the admin prominently shows the "Add user account" page as long as there is no user other than admin.

You seem to be arguing that it is dangerous to allow users to set things up for anonymous access. I do not agree at all. Anonymous read/write access is perfectly reasonable in many scenarios, and has a long history.

Jermolene avatar Oct 15 '24 10:10 Jermolene

You seem to be arguing that it is dangerous to allow users to set things up for anonymous access. I do not agree at all. Anonymous read/write access is perfectly reasonable in many scenarios, and has a long history.

As long as the servers are used on private networks only. But that's not what our users request. They request access from everywhere. So they want a simple way to connect their private servers to the internet.

Connecting servers with valuable data to the internet is dangerous. See: https://techcrunch.com/2024/10/14/2024-in-data-breaches-1-billion-stolen-records-and-rising/

Hacking vulnerable devices is simple. The Mirai botnet, where the source code is here at github, only uses 64 hardcoded passwords to hijack 600.000 devices. See: https://blog.cloudflare.com/inside-mirai-the-infamous-iot-botnet-a-retrospective-analysis/ -- IMO this is a strong proof, that users do not enable security if they are not forced to. The article at cloudflare is from 2017 and Mirai should not be a thread anymore.

It's easy to repeat catchphrases like "secure by default" but I don't think it's very useful without some critical analysis.

That's right and we should think a bit more about it. https://www.cisa.gov/resources-tools/resources/secure-by-design and https://www.cisa.gov/sites/default/files/2023-10/SecureByDesign_1025_508c.pdf

The PDF above is mainly addressing companies, but I think the principles are worth considering.

Especially in a TiddlyWiki context of education, where we are really strong: See: https://www.cisa.gov/sites/default/files/2023-08/K-12_Acquisition_Guidance.pdf (only 2 pages)

pmario avatar Oct 15 '24 12:10 pmario

@Jermolene @pmario Do we all agree to build interactive UI so that users can configure when they start the project? Or what approach would we proceed on?

webplusai avatar Oct 15 '24 14:10 webplusai

As long as the servers are used on private networks only. But that's not what our users request. They request access from everywhere. So they want a simple way to connect their private servers to the internet.

That is one of the things that users want. But it doesn't really advance your case.

Connecting servers with valuable data to the internet is dangerous. See: https://techcrunch.com/2024/10/14/2024-in-data-breaches-1-billion-stolen-records-and-rising/

It is also ubiquitous. I really don't see the relevance of this comment.

We're talking here about the onboarding experience of installing and setting up a new server. "Valuable data" only enters the picture once it is set up as wanted.

Hacking vulnerable devices is simple. The Mirai botnet, where the source code is here at github, only uses 64 hardcoded passwords to hijack 600.000 devices. See: https://blog.cloudflare.com/inside-mirai-the-infamous-iot-botnet-a-retrospective-analysis/ -- IMO this is a strong proof, that users do not enable security if they are not forced to. The article at cloudflare is from 2017 and Mirai should not be a thread anymore.

What is the relevance of this observation?

That's right and we should think a bit more about it. https://www.cisa.gov/resources-tools/resources/secure-by-design and https://www.cisa.gov/sites/default/files/2023-10/SecureByDesign_1025_508c.pdf

Again, what is the relevance of those PDFs to your argument?

Especially in a TiddlyWiki context of education, where we are really strong: See: https://www.cisa.gov/sites/default/files/2023-08/K-12_Acquisition_Guidance.pdf (only 2 pages)

Again, what bearing does this have on your argument.

The position at the moment is that I proposed a way forward for @webplusai and you've raised a vague concern about "security by default" without providing any actionable feedback to the proposal.

Jermolene avatar Oct 15 '24 14:10 Jermolene

@Jermolene @pmario Do we all agree to build interactive UI so that users can configure when they start the project? Or what approach would we proceed on?

I think @pmario's concerns need to be taken out of the critical path of your work. I would suggest proceeding along the lines I proposed. Remember that we're trying to do minimal work to be able to merge this with a reasonable user experience. Making it anonymous access by default matches the current configuration and so is categorically not a problem. We can't hold up merging your work until some hypothetical point has been reached, we need to be practical.

Jermolene avatar Oct 15 '24 14:10 Jermolene

@Jermolene so just to confirm, we will proceed on this way. Right? The best I can think of is an interactive wizard that kicks in at startup to ask the user how they want their server configured.

webplusai avatar Oct 15 '24 14:10 webplusai

@Jermolene so just to confirm, we will proceed on this way. Right? The best I can think of is an interactive wizard that kicks in at startup to ask the user how they want their server configured.

Again, we need to look at this through the lens of what is the path that allows us to merge this PR as swiftly as possible. I think it's perfectly OK for the out-of-the-box experience to be anonymous read/write access, with either a web or command line path to setting up security, whichever will be quickest to build.

Jermolene avatar Oct 15 '24 14:10 Jermolene

I have implemented a flag that allows the app to toggle on authentication. By default, when the user starts the app using the npm run start command, it runs without requiring authentication. But when the app is started using the npm run start -- --mws-use-auth command then authentication is turned on.

Would you check and let me know if this is what you want @Jermolene ?

webplusai avatar Oct 16 '24 10:10 webplusai

I have implemented a flag that allows the app to toggle on authentication. By default, when the user starts the app using the npm run start command, it runs without requiring authentication. But when the app is started using the npm run start -- --mws-use-auth command then authentication is turned on.

Would you check and let me know if this is what you want @Jermolene ?

I don't think that is the right way to handle anonymous access. Users will need to be able to configure wikis independently, so that one may have anonymous read access, one may have both anonymous read and write access and another may be entirely access controlled.

I think that means that we can't have a server-wide switch that switches authentication on or off. We just need an access control system that allows a resource to be marked as permitting or blocking anonymous access.

Jermolene avatar Oct 16 '24 11:10 Jermolene