-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
This commit further refactors existing work to make windows LDAP and … #18021
This commit further refactors existing work to make windows LDAP and … #18021
Conversation
135ef5a
to
7766dcb
Compare
lib/auth/windows/ldap.go
Outdated
return sb.String() | ||
} | ||
|
||
// ComputerAttributes are the attributes we fetch when discovering |
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.
These can probably stay in windows_server.go since they are tailored towards discovering computers for desktop access.
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 see your point there. That makes sense.
7766dcb
to
4dde4e3
Compare
func NewCertificateAuthority(clusterName string, | ||
ldapConfig LDAPConfig, | ||
ldapClient *LDAPClient, | ||
ap auth.WindowsDesktopAccessPoint, | ||
log logrus.FieldLogger) *CertificateAuthority { |
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.
nit: Why not just:
func NewCertificateAuthority(clusterName string, | |
ldapConfig LDAPConfig, | |
ldapClient *LDAPClient, | |
ap auth.WindowsDesktopAccessPoint, | |
log logrus.FieldLogger) *CertificateAuthority { | |
func NewCertificateAuthority(cfg CertificateAuthorityConfig) (*CertificateAuthority, error) { | |
if err := cfg.ValidateAndSetDefault(); err != nil { | |
return nil, err | |
} | |
return CertificateAuthority{cfg: cfg}, nil | |
} |
where clusterName
and lc
can be part of CertificateAuthorityConfig
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.
Sounds very reasonable.
lib/srv/desktop/windows_server.go
Outdated
@@ -351,6 +364,8 @@ func NewWindowsService(cfg WindowsServiceConfig) (*WindowsService, error) { | |||
s.cfg.Log.Infoln("desktop discovery via LDAP is disabled, set 'base_dn' to enable") | |||
} | |||
|
|||
s.ca = windows.NewCertificateAuthority(s.clusterName, s.cfg.LDAPConfig, s.lc, s.cfg.AccessPoint, s.cfg.Log) |
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.
nit: What would you cay about renaming NewCertificateAuthority
to something like NewStoreClient
?
At first glance ca
and NewCertificateAuthority
naming is a bit misleading for a reader and indicatate the ca contains some kind of CertificateAuthority
object but actually the ca is a windows store client that allows to update ca in that store.
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 that's a good idea.
a84d1e1
to
70c6fed
Compare
f9213f7
to
a54b6f2
Compare
This commit further refactors existing work to make windows LDAP and Certificate services available to other parts of Teleport. In particular, this PR moves the majority of the LDAP and CA services from windows_server.go into the windows package and into a new structure CertificateAuthority
a54b6f2
to
6771e20
Compare
This commit further refactors existing work to make windows LDAP and Certificate services available to other parts of Teleport. In particular, this PR moves the majority of the LDAP and CA services from windows_server.go into the windows package and into a new structure CertificateAuthority
This commit further refactors existing work to make windows LDAP and Certificate services available to other parts of Teleport. In particular, this PR moves the majority of the LDAP and CA services from windows_server.go into the windows package and into a new structure CertificateAuthority
This commit further refactors existing work to make windows LDAP and Certificate services available to other parts of Teleport. In particular, this PR moves the majority of the LDAP and CA services from windows_server.go into the windows package and into a new structure CertificateAuthority
…Certificate services available to other parts of Teleport. In particular, this PR moves the majority of the LDAP and CA services from windows_server.go into the windows package and into a new structure CertificateAuthority