-
Notifications
You must be signed in to change notification settings - Fork 1
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[MVP]integrate LDAP as external auth service #2
Conversation
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.
Looks great, please add a CI for it. it could be run the existing test cases in github action, as the first step.
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.
seems the github action will only run after this PR merged? Let's finished the smalls comments, and merged it as the first version. Thanks!
It will run in my repository, and you can find it there. |
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.
just a few comments, otherwise, I think it good for the first version.
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.
good enough for the first version, may add more comments, please fix in other PRs.
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.
please also fix the TODOs, thanks!
go-version: 1.19 | ||
|
||
- name: build | ||
run: make build |
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.
need run tests.
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.
Also, need a better way to test other modes, now we could only test with simple static configuration.
we should test with different configuration modes, eventually, not a very high priority.
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 will fix this later.
bind_dn: | ||
bind_password: | ||
# if the filter is set, the filter application will run in search mode. | ||
filter: |
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.
better add an example config as demo, in comment could be good enough.
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.
OK
bind_password: | ||
# if the filter is set, the filter application will run in search mode. | ||
filter: | ||
timeout: 60 |
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.
add comment: unit is second.
} | ||
|
||
func (p *parser) Merge(parent interface{}, child interface{}) interface{} { | ||
panic("TODO") |
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.
this TODO need to be addressed.
func dial(config *config) (*ldap.Conn, error) { | ||
return ldap.DialURL( | ||
// TODO: support TLS | ||
fmt.Sprintf("ldap://%s:%d", config.host, config.port), |
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.
use fmt
for debug log is not a good idea, eventually. please use the HttpLog in filter.callbacks.
func newLdapClient(config *config) (*ldap.Conn, error) { | ||
client, err := dial(config) | ||
if err != nil { | ||
fmt.Println("ldap dial error: ", err) |
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.
ditto and somewhere else.
see #1 . This MVP version of LDAP integration as an external authentication service does not currently support TLS (Transport Layer Security). Other detailed explanations can be found in the README.md file.