[pve-devel] RE : RE : RE : pve-devel Digest, Vol 132, Issue 53

[wb]

Hello Thomas,

Thanks for all these clarifications, I was able to move forward and put a V0.

https://github.com/jbsky/proxmox-saml2-auth/commit/75c9f089170aacf02370ab86bfa8c75fe0c76a6a.diff

Instead of creating a POST /access/saml endpoint, I finally reused the POST /access/ticket (thanks for your comment ;)

The important points remaining:
➢ For the login page, we need to create links to each SAML "realm" create, we should have <a href ...> links, there, there is nothing.

➢ Correctly check the saml request id in the authentication part of the SAML class from the user cookie instead of storing it directly on the server (that's what I did).
I had no problem creating a user/client side cookie (e.g. chrome), however, I have trouble retrieving it and providing it to the endpoints as a parameter.
Is it possible to pass a cookie from the client to an endpoint? 
That would be much cleaner, we would be at the target of what is recommended.

➢ I created an attribute realm_proxmox on the IdP side to provide the realm to the endpoint "/access/ticket" , same as the previous point, if the client could provide it with these cookies.

I will prepare the git-mail for you tomorrow or the day after at the earliest.

Sincelery,

Julien BLAIS

De : Thomas Lamprecht
Envoyé le :mercredi 26 mai 2021 08:22
À : wb; Proxmox VE development discussion
Objet :Re: RE : [pve-devel] RE : pve-devel Digest, Vol 132, Issue 53

Hi,

On 25.05.21 22:50, wb wrote:
> Sorry, the first mail went a little too fast
> 
>> running does not mean working...
> Ok, but starting from an installation, I doubt :).
> 

But you already started to patch around, so we just cannot know for sure - was just
mentioning that to cover more possibilities :-)


> systemctl status pve-cluster 
> 
> 
> 
> touch /etc/pve/foo
> findmnt /etc/pve
> 

Look OK, but please paste such things inline the next time, I and most others here
on this list find pictures of plain text rather needlessly hard to deal with.

> 
> I finally found the problem that comes from the read permission on the /etc/pve/priv/authkey.key file, probably because www-data is reading instead of root.
> 

I mean yes, it still is a filesystem so read/write need to adhere to the standard
POSIX file permissions.

Quoting from our documentation:

> All files and directories are owned by user root and have group www-data.
> Only root has write permissions, but group www-data can read most files.
> Files below the following paths:> 
> /etc/pve/priv/
> /etc/pve/nodes/${NAME}/priv/
> 
> are only accessible by root.

-- https://pve.proxmox.com/pve-docs/chapter-pmxcfs.html#_file_access_rights

If a API method needs to access the `priv` path it needs to set the
`protected => 1` "flag" in the API-method schema definition, that way it will proxied
to the local-only API daemon running as root.


> By moving it to /etc/pve/authkey.key, I finally manage to create the ticket and thus, to do SSO.
> An analysis of the problem in my code would be welcome.
> 
> There will still be 2 points to be clean (not counting the anomaly due to the right in reading) :
> ➢ First point, create links in the login page instead of putting saml authentication realm in the list.
> ➢ Check the saml request id properly in the authentication part of the SAML class, (this is really not the case)
> 
> Thank you in advance,
> 
> Attached is the patch.

This is a combined patch over multiple repositories, further having it as attachment
makes replying (reviewing) hard to do on the list.

Please check out the following resources:
* Developer docs, especially the CLA, and preparing/sending patches section:
  https://pve.proxmox.com/wiki/Developer_Documentation

* Our coding style guides, didn't looked to close to actually see violations so rather
  mentioning it for completeness sake:
  - https://pve.proxmox.com/wiki/Perl_Style_Guide
  - https://pve.proxmox.com/wiki/Javascript_Style_Guide

  Note, we're well aware that not all of the existing code holds up to those standards
  (yet), that's still no real excuse for new code to adapt to it, as much as the surrounding
  code style allows.

Just two short comments regarding the patch, as I did not checked that out in full:

1. I'm not sure why you need to duplicate API calls, especially the `post_saml` one.
2. It also seems you missed including the diff for the new `PVE::Auth::SAML` module.

If you want a real review I'd suggest to send out what you have now as real patche
series via `git send-email`, ideally adding some reasoning for why things are done
that way - making review much easier - thanks!

cheers,
Thomas