-
Notifications
You must be signed in to change notification settings - Fork 252
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
Support for rfc3062 Password Modify, closes #163 #178
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -539,6 +539,51 @@ def modify(args) | |
pdu | ||
end | ||
|
||
## | ||
# Password Modify | ||
# | ||
# http://tools.ietf.org/html/rfc3062 | ||
# | ||
# passwdModifyOID OBJECT IDENTIFIER ::= 1.3.6.1.4.1.4203.1.11.1 | ||
# | ||
# PasswdModifyRequestValue ::= SEQUENCE { | ||
# userIdentity [0] OCTET STRING OPTIONAL | ||
# oldPasswd [1] OCTET STRING OPTIONAL | ||
# newPasswd [2] OCTET STRING OPTIONAL } | ||
# | ||
# PasswdModifyResponseValue ::= SEQUENCE { | ||
# genPasswd [0] OCTET STRING OPTIONAL } | ||
# | ||
# Encoded request: | ||
# | ||
# 00\x02\x01\x02w+\x80\x171.3.6.1.4.1.4203.1.11.1\x81\x100\x0E\x81\x05old\x82\x05new | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍 thanks for this documentation! |
||
# | ||
def password_modify(args) | ||
dn = args[:dn] | ||
raise ArgumentError, 'DN is required' if !dn || dn.empty? | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍 I like this abstraction! |
||
ext_seq = [Net::LDAP::PasswdModifyOid.to_ber_contextspecific(0)] | ||
|
||
unless args[:old_password].nil? | ||
pwd_seq = [args[:old_password].to_ber(0x81)] | ||
pwd_seq << args[:new_password].to_ber(0x82) unless args[:new_password].nil? | ||
ext_seq << pwd_seq.to_ber_sequence.to_ber(0x81) | ||
end | ||
|
||
request = ext_seq.to_ber_appsequence(Net::LDAP::PDU::ExtendedRequest) | ||
|
||
message_id = next_msgid | ||
|
||
write(request, nil, message_id) | ||
pdu = queued_read(message_id) | ||
|
||
if !pdu || pdu.app_tag != Net::LDAP::PDU::ExtendedResponse | ||
raise Net::LDAP::ResponseMissingError, "response missing or invalid" | ||
end | ||
|
||
pdu | ||
end | ||
|
||
#-- | ||
# TODO: need to support a time limit, in case the server fails to respond. | ||
# Unlike other operation-methods in this class, we return a result hash | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -74,6 +74,7 @@ class Error < RuntimeError; end | |
attr_reader :search_referrals | ||
attr_reader :search_parameters | ||
attr_reader :bind_parameters | ||
attr_reader :extended_response | ||
|
||
## | ||
# Returns RFC-2251 Controls if any. | ||
|
@@ -120,7 +121,7 @@ def initialize(ber_object) | |
when UnbindRequest | ||
parse_unbind_request(ber_object[1]) | ||
when ExtendedResponse | ||
parse_ldap_result(ber_object[1]) | ||
parse_extended_response(ber_object[1]) | ||
else | ||
raise LdapPduError.new("unknown pdu-type: #{@app_tag}") | ||
end | ||
|
@@ -180,6 +181,29 @@ def parse_ldap_result(sequence) | |
end | ||
private :parse_ldap_result | ||
|
||
## | ||
# Parse an extended response | ||
# | ||
# http://www.ietf.org/rfc/rfc2251.txt | ||
# | ||
# Each Extended operation consists of an Extended request and an | ||
# Extended response. | ||
# | ||
# ExtendedRequest ::= [APPLICATION 23] SEQUENCE { | ||
# requestName [0] LDAPOID, | ||
# requestValue [1] OCTET STRING OPTIONAL } | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍 great docs. |
||
|
||
def parse_extended_response(sequence) | ||
sequence.length >= 3 or raise Net::LDAP::PDU::Error, "Invalid LDAP result length." | ||
@ldap_result = { | ||
:resultCode => sequence[0], | ||
:matchedDN => sequence[1], | ||
:errorMessage => sequence[2] | ||
} | ||
@extended_response = sequence[3] | ||
end | ||
private :parse_extended_response | ||
|
||
## | ||
# A Bind Response may have an additional field, ID [7], serverSaslCreds, | ||
# per RFC 2251 pgh 4.2.3. | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3,7 +3,7 @@ objectClass: olcGlobal | |
cn: config | ||
olcPidFile: /var/run/slapd/slapd.pid | ||
olcArgsFile: /var/run/slapd/slapd.args | ||
olcLogLevel: none | ||
olcLogLevel: -1 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👌 |
||
olcToolThreads: 1 | ||
|
||
dn: olcDatabase={-1}frontend,cn=config | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,80 @@ | ||
require_relative '../test_helper' | ||
|
||
class TestPasswordModifyIntegration < LDAPIntegrationTestCase | ||
def setup | ||
super | ||
@ldap.authenticate 'cn=admin,dc=rubyldap,dc=com', 'passworD1' | ||
|
||
@dn = 'uid=modify-password-user1,ou=People,dc=rubyldap,dc=com' | ||
|
||
attrs = { | ||
objectclass: %w(top inetOrgPerson organizationalPerson person), | ||
uid: 'modify-password-user1', | ||
cn: 'modify-password-user1', | ||
sn: 'modify-password-user1', | ||
mail: '[email protected]', | ||
userPassword: 'passworD1' | ||
} | ||
unless @ldap.search(base: @dn, scope: Net::LDAP::SearchScope_BaseObject) | ||
assert @ldap.add(dn: @dn, attributes: attrs), @ldap.get_operation_result.inspect | ||
end | ||
assert @ldap.search(base: @dn, scope: Net::LDAP::SearchScope_BaseObject) | ||
|
||
@auth = { | ||
method: :simple, | ||
username: @dn, | ||
password: 'passworD1' | ||
} | ||
end | ||
|
||
def test_password_modify | ||
assert @ldap.password_modify(dn: @dn, | ||
auth: @auth, | ||
old_password: 'passworD1', | ||
new_password: 'passworD2') | ||
|
||
assert @ldap.get_operation_result.extended_response.nil?, | ||
'Should not have generated a new password' | ||
|
||
refute @ldap.bind(username: @dn, password: 'passworD1', method: :simple), | ||
'Old password should no longer be valid' | ||
|
||
assert @ldap.bind(username: @dn, password: 'passworD2', method: :simple), | ||
'New password should be valid' | ||
end | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would expect that we also test the new password works and the old password fails as a sanity check. |
||
|
||
def test_password_modify_generate | ||
assert @ldap.password_modify(dn: @dn, | ||
auth: @auth, | ||
old_password: 'passworD1') | ||
|
||
generated_password = @ldap.get_operation_result.extended_response[0][0] | ||
|
||
assert generated_password, 'Should have generated a password' | ||
|
||
refute @ldap.bind(username: @dn, password: 'passworD1', method: :simple), | ||
'Old password should no longer be valid' | ||
|
||
assert @ldap.bind(username: @dn, password: generated_password, method: :simple), | ||
'New password should be valid' | ||
end | ||
|
||
def test_password_modify_generate_no_old_password | ||
assert @ldap.password_modify(dn: @dn, | ||
auth: @auth) | ||
|
||
generated_password = @ldap.get_operation_result.extended_response[0][0] | ||
|
||
assert generated_password, 'Should have generated a password' | ||
|
||
refute @ldap.bind(username: @dn, password: 'passworD1', method: :simple), | ||
'Old password should no longer be valid' | ||
|
||
assert @ldap.bind(username: @dn, password: generated_password, method: :simple), | ||
'New password should be valid' | ||
end | ||
|
||
def teardown | ||
@ldap.delete dn: @dn | ||
end | ||
end |
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.
@mtodd I have moved the value into universal, context specific constructed will not work because it starts at 160 and it's not a primitive.
To be honest I don't know why this stuff is written the way it is when all that is resulted is an array with 256 length with values at index with the types, why not just one simple flat dict.
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.
@mynameisrufus I agree 100% that the parsing behavior needs to be refactored.
I'm uncomfortable accepting this as is without a better understanding of the specific value we're adding here. This change doesn't align with my understanding of the
ExtendedResponse
mechanics. I don't want to accept something that makes it work but isn't clear why because it will likely have unintended/unaccounted for consequences.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 do you mean by 'specific value'?, to me the specific value is in the title of the PR, support for modify password http://tools.ietf.org/html/rfc3062.
What is your understanding of the mechanics of
ExtendedResponse
, if you could share with me your understanding I would be happy to make changes to this PR.I have invested considerable time in understanding this codebase and have addressed every point raised by yourself and @jch. I have sniffed packets, groked Open LDAP source and the source of many other libs in order to comprehensively document and explain each change I have made, I am committed to delivering this feature and like I said, I am happy to address any concerns you have or explain the workings of LDAP or this library. I fully understand that you have inherited this codebase so I understand the fear you may have in accepting changes to this lib.