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
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
