search

mikeGEINE / FPauth

Authentication framework for OCaml Dream web-framework
https://mikegeine.github.io/FPauth/
MIT License
16 stars 1 forks source link

Doesn't support 5.1.0 #2

Open joelatschool opened 7 months ago

joelatschool commented 7 months ago

FPauth currently conflicts with ocaml-base-compiler.5.1.0. I can't quite tell why; officially, it's because it uses naked pointers in FPauth-core, but I cannot determine where those naked pointers are. The only other barrier to this package being up-to-date is that it uses twostep which does not support recent versions of mirage-crypto-rng, and thus cannot be installed alongside dream.1.0.0~alpha5.

mikeGEINE commented 7 months ago

Oh, hi there! Yes, it is mentioned in one of FPauth sub-packages, FPauth-core, that it conflicts with base-nnp. However, that always bugged me and I wanted to fix that. At the time of the package release I never found out why that naked pointer check failed.

I'll try to look into this once more in the coming days. If you find out something on this issue to act on, I would be grateful.

joelatschool commented 7 months ago

For what it's worth, after cloning into FPauth, manually removing the conflicts from the .opam file, setting up a local switch with invariant 4.14.1+options ocaml-option-nnpchecker and running dune test, all the tests pass without any complaints from nnpchecker. Do you remember the commit where the naked pointer check failed? I wonder if its due to a dependency that failed the check at the time but no longer does.

mikeGEINE commented 7 months ago

The thing is that the package was developed and tested with OCaml 4.12, if I'm not mistaken (definitely before general release of OCaml 5.0). And the check failed only during CI when pull requesting to opam, as they already checked compatibility with 5.0. So I can say that the check failed on the most recent commit. At that time we decided just to leave the package as is and marked, that it is not supported by OCaml 5.0.

So, I think the issue is connected to changes in 5.0, but I never looked any deeper in this.

joelatschool commented 7 months ago

I've found the discussion from the time but I can't determine what caused the failure because I can't locate the CI logs to replicate it: https://github.com/ocaml/opam-repository/pull/21224#issuecomment-1103819345 I suspect that it was a dependency that has since been updated, because I don't think you should have naked pointers in your code unless you're using Obj or Marshal or something like that.

joelatschool commented 7 months ago

Also, just figured out how to set up a local opam switch so that I can test FPauth-core on 5.1.0 without it conflicting with FPauth-strategies (which needs < 5.0 bc of twostep) and all the tests pass.

mikeGEINE commented 7 months ago

So, I tried to recreate the issue with nnpchecker in opam docker images, which are used for their CI. Aaaand... Nothing :) I tested it in Debian 12 based images running OCaml 4.12 and 4.14 (it actually failed on 4.14 when the package was published), nothing. I tried using minimal supported versions of dependencies, still nothing. I used OCaml 5.1 image, no problems with FPauth-core. I start to think the problem at that time was not with the package. Maybe it was an issue with a dependency, maybe something in images, but now I don't see those errors.

What do I suggest? I can remove the conflicts section and release it as FPauth-core.1.0.1. Tests are passing, so I think it should work. core does not depend on strategies, they can be installed independently, so there is nothing breaking. At the same time it might be good to update strategies, so that they also work with OCaml > 5.0. So, I'll check this out, and when (or if) there is a solution to this, there will be a new version of strategies, and it will be released together with core.

Thank you for bringing it up! If you find out more on this or other issues, feel free to contact!

joelatschool commented 7 months ago

Thanks! Unfortunately, twostep, which strategies depends on for TOTP, seems unmaintained: it depends on mirage-crypto-rng which has undergone a minor but breaking syntax change, and despite a PR to fix this being open for around a year it was never merged in and eventually closed by the author. This makes it impossible to install twostep alongside versions of dream which support OCaml 5.

As such, I think that it probably is best to update and release core separately. If possible, it might also useful to split off the password authentication strategy separately from TOTP, as it's probably the most common one and I think there aren't any issues with updating it to OCaml 5.

By the way, this isn't really related but I haven't actually gotten to use this package yet and I'm a little unclear what the intended usage is. In particular, I'm confused by FPauth_strategies.Password.MODEL.encrypted_password returning a string option and not a string option Dream.promise or string Dream.promise. If User.t is expected to actually store the hashed password of the user, does that mean that the hashed password of the user is stored in the session? Is all authentication data for a user intended to be retrieved at the start by identificate?

mikeGEINE commented 7 months ago

I see, there is some confusion regarding MODEL. I couldn't find any ORM solutions in OCaml by that time, so I had to come up with some requirements for authentication to work, though they may be far from perfect. The idea is that identificate (seems it should be identify or smth, but okay) is needed to define, which user is about to be authenticated, and it should return some user representation to work later with during request handling. Password strategy then requires that there should be a way to retrieve a password of the identified user somehow and return it as string option (I don't remember why option, maybe just in case something fails). It does not require MODEL.t to contain this password, however, this function can have various implementations based on your particular case. Though I, probably, didn't think it through: if it needs to be retrieved from a database, for example, we'll have to wait for the result. So it seems that you're right, it would be better to return Lwt.t from encrypted_password.

However, this doesn't necessarily mean that we store all authentication data in session. MODEL needs to have a serialize and deserialize functions, which can omit data to store user in session securely. For example, serialize can return only user id, and deserialize will have to retrieve user from DB by id. But this should be wrapped in Lwt.t, which now is not required.

Sooo, yeah, this is better to be updated too.