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

[Thomas Lamprecht <t.lamprecht@proxmox.com>]

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