SASL / SCRAM-SHA-256 Authentication

[Gustl22]

Closes stablekernel/postgresql-dart#145

I used the code base from the mongo-dart project.

There are a few TODOs, but only to ensure the correctness of the code.

[isoos]

I'm wondering: is there any tests in mongo_dart that could be ported too? This seems to be an awful lot of code that will go without testing...

[isoos]

It would be worth to consider to extract the SASL and related logic into a separate library that could be used by multiple packages. Maybe @vadimtsushko or @giorgio would be interested in that?

[Pacane]

I did write that part a while back and used test cases from the RFC IIRC, that test case was also used in the JS mongo implementation

[Gustl22]

Thanks a lot for the fast review. Sounds like a nice idea with the separate library, although I would propose the mongo-dart team to host the library, as they wrote the code, if they agree. @isoos I think you mean @giorgiofran

I'll have a look at these tests, if a separate lib isn't an option :)
https://github.com/mongo-dart/mongo_dart/blob/main/test/authentication_test.dart
Also I'm quite new to dart, so may need some time to dive deeper into mocking / testing partial code fragments.

@Pacane thank you for your feedback on this!

[giorgiofran]

A separate lib could be interesting. Please, consider that I have inherited this part, so I should analyze it before doing such a job. It would be nice if @Pacane could cooperate in this effort but I do not know if he has the time to do it.

[danielgomezrico]

I tested it and it works nice!

Without it, I wasn't able to run some integration tests for sanity because we are in the middle of a migration to flutter 2 :)

Thank you, you saved my life, +1 to merge

[isoos]

So, any takers for a separate library? @Pacane @giorgiofran @Gustl22?
I'd me more comfortable merging this if there were more test to it too...

[Pacane]

I currently do not have enough time to create a separate package for this, however I can give some insight of how I'd implement it. As for the tests in mongo we just have an end-to-end test for this part.

If we were to extract this into a separate package I'd probably try to make the algorithm generic enough so that one could pass a generic connection to the SaslAuthenticator/Conversation. The same effect could also be achieved by using closures in those classes to provide the mechanisms to talk to the DB/parse the results in order for the SaslAuthenticator to continue the SASL conversation.

Any thoughts?

[Gustl22]

I can attempt to create a new lib and test it for postgres. But I would be glad, if someone (mongo-dart?) can host and maintain it then.

[giorgiofran]

A new lib can be hosted on mongo-dart. I have not so much time to prepare a new repository, If you can prepare it, we will publish it.

[Gustl22]

Hey there, I now published a repo with the lib (and kept the git log from the related mongo_dart files). @Pacane I would be happy if you write your opinion on the implementation. I was not quite sure how to achieve it with closures as the application has to wrap around the auth message anyways. But every improvement is welcome, of course.

I also uploaded my own tests and it worked e.g. with this config from node-postgres (see here). At xdg-go are more test cases available.

@giorgiofran if you're happy with the repo, you could reupload the git repo without forking, then I'll delete my repo and we can improve the lib at mongo-dart.

Of course you can take your time and may also migrate / test for the mongo_dart lib.

[giorgiofran]

I created the new repo:
For testing set the following in your pubspec:

 sasl_scram:
    git:
      url: "https://github.com/mongo-dart/sasl_scram.git"    
      ref: main

I simply uploaded your work. To use the new package I needed to adapt the mongo_dart sources. I did some work this weekend, but I had no time to test neither the changes nor the new package. Maybe next w.e. I will find some time.

[isoos]

I think this looks great, many thanks for everyone who got involved and helped to make this new package happen! :) Once it gets published I'll be happy to merge and publish this too.

@Gustl22: Could you please also update the CHANGELOG.md?

[Gustl22]

Sure, I'll update the log, when as soon as lib is ready :)

@giorgiofran Note that I've disabled the password digest, which may needs another param in the authenticator in order to work in mongo-dart project.

[giorgiofran]

@Gustl22 I have published the package.
As you have pointed out, I had to do a couple of changes in order to make MongoDb work with the new package.
You can see it in the repository, but I highlight you the main changes:
1st in scram_mechanism.dart

String username;
if (specifyUsername) {
  username = 'n=${prepUsername(credential.username!)}';
} else {
 username = 'n=*';
}

I had to set a new parameter (optional, default false) because MongoDb requires also the user name. No way of make it work without It! This should not impact your code as the default is false.

2nd client_first.dart

String passwordDigest;
if (passwordDigestResolver != null) {
  passwordDigest = passwordDigestResolver(credential);
} else {
  passwordDigest = Saslprep.saslprep(credential.password!);
}

Here again I use a specific function to calculate the password digest. The default is null, so for you almost nothing is changed.
The only difference is that I added a password sanitize method from the Saslprep package (rfc4013).
In my tests it works, but you should try to see if it is OK for you also.

[Gustl22]

@giorgiofran Thank you VERY much for attending and publishing the library!
I haven't noticed any regression, so from my side all is ready now. None the less, if you need to do further changes, I can adapt this PR accordingly.
You may also want to add the link in the GH-repo to the published package: https://pub.dev/packages/sasl_scram (I'm not permitted to change the repo link 😁 ).

@isoos I've updated the Changelog, so this may gets into the next release together with the other open PRs ;D
Thank you all for helping and contributing to this issue!

[isoos]

A few nits left, otherwise looks good to me!

[isoos]

All-in-all, I'm really happy about this change, but I want to do a quick final review tomorrow.

[isoos]

I think there is only one request left. Let me know if you don't have time for it, I can do it in a follow-up commit.

[isoos]

I think there is no reason to make this mutable on the public API, and maybe, there is no need to expose it at all.

• Let's make this private: AuthenticationScheme _authenticationScheme = AuthenticationScheme.SCRAM_SHA_256;
• Let's pass this as an additional parameter to createAuthenticator, the only caller point will have access to the private field.

[Gustl22]

I think it depends: I don't know if there are actually auth mechanisms, where the user can decide which to use (e.g. md5 vs. plain text (?)). If not, then the authenticationScheme variable can be ditched totally and only work as param as the server communicates with the client which auth mechanism to choose.

You're always welcomed to change the code in my PRs, no need to ask if you're sure about something :)

[isoos]

This is a great PR, thanks @Gustl22 for seeing it through, @giorgiofran and others to help with the SASL library!