Support SAML authentication #29403
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -90,6 +90,8 @@ require ( | |
| github.com/quasoft/websspi v1.1.2 | ||
| github.com/redis/go-redis/v9 v9.5.1 | ||
| github.com/robfig/cron/v3 v3.0.1 | ||
| github.com/russellhaering/gosaml2 v0.9.1 | ||
|
There was a problem hiding this comment. I found a more popular package(crewjam/saml) that we can use. It looks like russellhaering/gosaml2 only supports HTTP POST binding, but crewjam/saml supports both HTTP Redirect binding and HTTP POST binding. There was a problem hiding this comment. if you make a pull against my feature branch we can test that lib :) There was a problem hiding this comment. PS: it's no simple drop-in replacement :/ There was a problem hiding this comment. @Zettat123 the pull is not originally from me but was just resubmited for review. so @techknowlogick or @jackHay22 might know more about why thy have choosen the lib? There was a problem hiding this comment. @Zettat123 I'll added it to the followups is that ok or should we wait for the others to respond? There was a problem hiding this comment. I don't have a strong opinion here; if the additional features are useful this would be a good time to make the switch. There was a problem hiding this comment. @Zettat123 It looks like gosaml2 supports HTTP redirect binding. Let me know if I'm missing something. There was a problem hiding this comment. Found an issue related to the Redirect binding. I haven't looked into this issue, but it looks like there is a workaround. But I think this issue not being solved for a long time means that the project is not very active. There was a problem hiding this comment. I don't disagree but I think the There was a problem hiding this comment. I think at some point a saml lib is just completed and only needs version bumps if it has dependencys? so in this regards if there are no issues open anymore I'm fine with a inactive lib. here both repos still have open issues :/ |
||
| github.com/russellhaering/goxmldsig v1.3.0 | ||
| github.com/santhosh-tekuri/jsonschema/v5 v5.3.1 | ||
| github.com/sassoftware/go-rpmutils v0.3.0 | ||
| github.com/sergi/go-diff v1.3.1 | ||
|
|
@@ -143,6 +145,7 @@ require ( | |
| github.com/anmitsu/go-shlex v0.0.0-20200514113438-38f4b401e2be // indirect | ||
| github.com/asaskevich/govalidator v0.0.0-20230301143203-a9d515a09cc2 // indirect | ||
| github.com/aymerick/douceur v0.2.0 // indirect | ||
| github.com/beevik/etree v1.1.0 // indirect | ||
| github.com/beorn7/perks v1.0.1 // indirect | ||
| github.com/bits-and-blooms/bitset v1.13.0 // indirect | ||
| github.com/blevesearch/bleve_index_api v1.1.6 // indirect | ||
|
|
@@ -216,6 +219,7 @@ require ( | |
| github.com/imdario/mergo v0.3.16 // indirect | ||
| github.com/jbenet/go-context v0.0.0-20150711004518-d14ea06fba99 // indirect | ||
| github.com/jessevdk/go-flags v1.5.0 // indirect | ||
| github.com/jonboulle/clockwork v0.3.0 // indirect | ||
| github.com/josharian/intern v1.0.0 // indirect | ||
| github.com/kevinburke/ssh_config v1.2.0 // indirect | ||
| github.com/klauspost/pgzip v1.2.6 // indirect | ||
|
|
@@ -225,6 +229,7 @@ require ( | |
| github.com/magiconair/properties v1.8.7 // indirect | ||
| github.com/mailru/easyjson v0.7.7 // indirect | ||
| github.com/markbates/going v1.0.3 // indirect | ||
| github.com/mattermost/xml-roundtrip-validator v0.1.0 // indirect | ||
| github.com/mattn/go-colorable v0.1.13 // indirect | ||
| github.com/mattn/go-runewidth v0.0.15 // indirect | ||
| github.com/mholt/acmez v1.2.0 // indirect | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why would this be even a thing? It's like adding an option to not check the user's password in the login form...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@techknowlogick Can we remove this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's correct that it's incredibly insecure, as it bypasses any validation checks for the data that is sent to it, which is via HTTP post so it is accessible to the user, so the user can modify it in transit if validation checks are not added. It definitely shouldn't be enabled on production systems (after any setup has been done), or even if it does get enabled it should be disabled soon after (even if a system is in testing mode), and disabling access to the application expect to specific machines (likely done via reverse proxy IP allow list) during the time it is enabled.
This was added intentionally, though, for the reason that for debugging purposes in many other systems that support saml provide this as an option. It is to see if the saml is sending the required assertions and the application is parsing it into the correct user fields, and being able to debug that portion prior to seeing if it is a cert error.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While I see this feature as useful for development it still should not be an option for the end-user
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about marking it in the documentation as
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that would probably be ok in combination with warnings and confirm dialog that states risks when saving auth source
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this shouldn't be in the source code at all. If it stays, why not also have an option saying "Bypass password verification for all local users"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It won't help them if the error is so generic that they cannot tell if it's a missing assertion or invalid signature. This should be fixed with proper error handling that says "Assertion rejected, invalid signature" or "Assertion rejected, unable to determine username", not a "allow anyone access to any account on this Gitea instance by checking this one box"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't speak for all use-cases, but I have found this option (in other systems) to be incredibly helpful in the past. As not having control over both sides of the auth portion, being able to disable the signing portion, had allowed me to let the IAM team know to please check the certs, and it was a quick solve (this sadly happened to me more than once). 99% of the time issues I've run into have been cert related.
I'd disagree that adding a "bypass password check for all users" would be equal, as in that case you would already know if the database had failed, and for ldap the errors would be more specific to what type of tls error is happening.
That being said, I think I'm the only one advocating for the inclusion of this though, and so I'll suggest a poll that'll happen out over the next week (ending the morning of Mar25) to get a gauge from everyone:
😄 include the debug option, but make sure to have the strict language above regarding the warning, including a red banner for admins on each page, and possibly a cronjob to disable to setting after an hour.
🎉 remove the debug option fully
and whatever the results are, I'll follow them.
(if the poll can be updated to use different language, or some other adjustments, please respond below and I'll be happy to update).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess with the results tied (if we add my vote in) then I will go ahead with the removal.