[Freeipa-devel] [freeipa PR#317][comment] Unify password generation across FreeIPA

2017-01-06 Thread mbasti-rh
  URL: https://github.com/freeipa/freeipa/pull/317
Title: #317: Unify password generation across FreeIPA

mbasti-rh commented:
"""
Fixed upstream
master:
https://fedorahosted.org/freeipa/changeset/8db5b277a079fdfe5efbd7d49311f14489cee0e8
https://fedorahosted.org/freeipa/changeset/fb7c111ac13510609e2cba14ecf88cd2ed291a4b
"""

See the full comment at 
https://github.com/freeipa/freeipa/pull/317#issuecomment-270855325
-- 
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

[Freeipa-devel] [freeipa PR#317][comment] Unify password generation across FreeIPA

2017-01-06 Thread stlaz
  URL: https://github.com/freeipa/freeipa/pull/317
Title: #317: Unify password generation across FreeIPA

stlaz commented:
"""
I don't see any merge conflicts and the rebase was automatic so I don't see 
why, but ok. Just note that ipatool may be confused with me commiting 
@pspacek's commit as he was the author of the main code and I put it to work 
with the rest of IPA.
"""

See the full comment at 
https://github.com/freeipa/freeipa/pull/317#issuecomment-270853592
-- 
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

[Freeipa-devel] [freeipa PR#317][comment] Unify password generation across FreeIPA

2017-01-05 Thread mbasti-rh
  URL: https://github.com/freeipa/freeipa/pull/317
Title: #317: Unify password generation across FreeIPA

mbasti-rh commented:
"""
PR needs rebase
"""

See the full comment at 
https://github.com/freeipa/freeipa/pull/317#issuecomment-270705142
-- 
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

[Freeipa-devel] [freeipa PR#317][comment] Unify password generation across FreeIPA

2016-12-21 Thread mbasti-rh
  URL: https://github.com/freeipa/freeipa/pull/317
Title: #317: Unify password generation across FreeIPA

mbasti-rh commented:
"""
I would like to review this as well, so removing ACK to prevent pushing this to 
master
"""

See the full comment at 
https://github.com/freeipa/freeipa/pull/317#issuecomment-268577216
-- 
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

[Freeipa-devel] [freeipa PR#317][comment] Unify password generation across FreeIPA

2016-12-21 Thread pspacek
  URL: https://github.com/freeipa/freeipa/pull/317
Title: #317: Unify password generation across FreeIPA

pspacek commented:
"""
Works for me, server installation including DNSSEC worked fine.
"""

See the full comment at 
https://github.com/freeipa/freeipa/pull/317#issuecomment-268575899
-- 
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

[Freeipa-devel] [freeipa PR#317][comment] Unify password generation across FreeIPA

2016-12-12 Thread pspacek
  URL: https://github.com/freeipa/freeipa/pull/317
Title: #317: Unify password generation across FreeIPA

pspacek commented:
"""
Talk is cheap so here is the code!
~~~
import math
import string
import random


class TokenGenerator(object):
"""Tunable token generator."""
# without: = # ' " \ `
_special = '!$%&()*+,-./:;<>?@[]^_{|}~'
def_charsets = {
 'uppercase':
 {'chars': string.ascii_uppercase,
  'entropy': math.log(len(string.ascii_uppercase), 2)},
 'lowercase':
 {'chars': string.ascii_lowercase,
  'entropy': math.log(len(string.ascii_lowercase), 2)},
 'digits':
 {'chars': string.digits,
  'entropy': math.log(len(string.digits), 2)},
 'special':
 {'chars': _special,
  'entropy': math.log(len(_special), 2)},
}

def __init__(self, uppercase=0, lowercase=0, digits=0, special=0,
 min_len=0):
"""Specify character contraints on generated tokens.

Integer values specify minimal number of characters from given
character class and length.
Value False prevents given character from appearing in the token.

Example:
TokenGenerator(uppercase=3, lowercase=3, digits=0, special=False)

At least 3 upper and 3 lower case ASCII chars, may contain digits,
no special chars.
"""
self.rng = random.SystemRandom()
self.min_len = min_len
self.req_classes = dict(
uppercase=uppercase,
lowercase=lowercase,
digits=digits,
special=special
)

self.todo_charsets = self.def_charsets.copy()
# 'all' class is used when adding entropy to too-short tokens
# it contains characters from all allowed classes
self.todo_charsets['all'] = {'chars': ''.join(
[charclass['chars']
 for charclass_name, charclass
 in self.todo_charsets.items()
 if self.req_classes[charclass_name] is not False]
)}
self.todo_charsets['all']['entropy'] = math.log(
len(self.todo_charsets['all']['chars']), 2)

def __call__(self, req_entropy=128):
"""Generate token containing at least req_entropy bits.

req_entropy is minimal number of entropy bits attacker has to guess:
128 bits entropy: secure
256 bits of entropy: secure enough if you care about quantum computers

The generated token will fulfill containts specified in init.
"""
todo_entropy = req_entropy
password = ''
# Generate required character classes:
# The order of generated characters is fixed to comply with check in
# NSS function sftk_newPinCheck() in nss/lib/softoken/fipstokn.c.
for charclass_name in ['digits', 'uppercase', 'lowercase', 'special']:
charclass = self.todo_charsets[charclass_name]
todo_characters = self.req_classes[charclass_name]
while todo_characters > 0:
password += random.choice(charclass['chars'])
todo_entropy -= charclass['entropy']
todo_characters -= 1

# required character classes do not provide sufficient entropy
# or does not fulfill minimal length constraint
allchars = self.todo_charsets['all']
while todo_entropy > 0 or len(password) < self.min_len:
password += random.choice(allchars['chars'])
todo_entropy -= allchars['entropy']

return password


if __name__ == '__main__':
pwgen = TokenGenerator()
for i in range(100):
print(pwgen(256))
~~~

This code deterministically generates passwords. If character constraints are 
specified, the code might generate slightly longer passwords than the 
brute-force method. For example, 256 bit password with FIPS-compliant 
constrains (3 character classes) the difference is 41 vs. 40 characters. Given 
this different, I think that determinism trumphs shorter passwords.


Also, I think that it does not make sense to have `req_entropy` parameter in 
`__call__`. IMHO it makes sense to specify it along with other constrains in 
`__init__`.
"""

See the full comment at 
https://github.com/freeipa/freeipa/pull/317#issuecomment-266439544
-- 
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

[Freeipa-devel] [freeipa PR#317][comment] Unify password generation across FreeIPA

2016-12-12 Thread pspacek
  URL: https://github.com/freeipa/freeipa/pull/317
Title: #317: Unify password generation across FreeIPA

pspacek commented:
"""
The main problem here is that we are mixing two approaches together, i.e. 
entropy specification using bits + specification using character classes etc. 
which used to be means of expressing entropy requirements in a way 
understandable by ordinary users.

If I understand it correctly, the encoding here is just to please 
password-quality checkers because the real password strength should be provided 
by the `entropy` parameter.

So I propose to use character classes only for encoding but not during 
generation. That should simplify the code and make it easier to understand.
"""

See the full comment at 
https://github.com/freeipa/freeipa/pull/317#issuecomment-266397912
-- 
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

[Freeipa-devel] [freeipa PR#317][comment] Unify password generation across FreeIPA

2016-12-11 Thread stlaz
  URL: https://github.com/freeipa/freeipa/pull/317
Title: #317: Unify password generation across FreeIPA

stlaz commented:
"""
Correct me if I'm wrong here but I believe we're going for the scenario where 
the attacker has to guess the `xxx` bits of entropy and they know that they 
have to do it. We're not actually coding `xxx` bits of entropy as we need more 
entropy bits to get a sufficient result (hence `length = 
int(math.ceil(entropy_bits / math.log(len(self.chars), 2))`).
However! To the very first question of yours - unfortunately, there is a very 
small relation between the arguments in `__init__` and `__call__` as @tiran 
says:
> I'm not clever enough to come up with an algorithm to calculate the length 
> with additional restrictions. My gut feeling tells me that less than 15% per 
> character class (3 for upper/lower case and symbols, 1 for digit) should be 
> ok.

From the code you can see that if a certain class of characters should not be 
used, it's not accounted for in the calculation of the final length of the 
password but that's about it - if a further restriction is made (>1 character 
of the give character class), this restriction is also not accounted for. But 
since we're the ones who'll be using this token generator, I think we could 
live with this. There should be a warning in a docstring somewhere, though.

edit: Just realized - the code is wrong, the restriction to a certain class == 
None should just mean that the characters from the given class could but don't 
have to appear in the password (thus still need to be accounted for), the 
restriction of a certain class == 0 should mean the character should not appear 
in the password and should not be accounted for in the length calculation.
"""

See the full comment at 
https://github.com/freeipa/freeipa/pull/317#issuecomment-266362288
-- 
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

[Freeipa-devel] [freeipa PR#317][comment] Unify password generation across FreeIPA

2016-12-11 Thread stlaz
  URL: https://github.com/freeipa/freeipa/pull/317
Title: #317: Unify password generation across FreeIPA

stlaz commented:
"""
Correct me if I'm wrong here but I believe we're going for the scenario where 
the attacker has to guess the `xxx` bits of entropy and they know that they 
have to do it. We're not actually coding `xxx` bits of entropy as we need more 
entropy bits to get a sufficient result (hence `length = 
int(math.ceil(entropy_bits / math.log(len(self.chars), 2))`).
However! To the very first question of yours - unfortunately, there is a very 
small relation between the arguments in `__init__` and `__call__` as @tiran 
says:
> I'm not clever enough to come up with an algorithm to calculate the length 
> with additional restrictions. My gut feeling tells me that less than 15% per 
> character class (3 for upper/lower case and symbols, 1 for digit) should be 
> ok.
From the code you can see that if a certain class of characters should not be 
used, it's not accounted for in the calculation of the final length of the 
password but that's about it - if a further restriction is made (>1 character 
of the give character class), this restriction is also not accounted for. But 
since we're the ones who'll be using this token generator, I think we could 
live with this. There should be a warning in a docstring somewhere, though.
"""

See the full comment at 
https://github.com/freeipa/freeipa/pull/317#issuecomment-266362288
-- 
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

[Freeipa-devel] [freeipa PR#317][comment] Unify password generation across FreeIPA

2016-12-11 Thread stlaz
  URL: https://github.com/freeipa/freeipa/pull/317
Title: #317: Unify password generation across FreeIPA

stlaz commented:
"""
Correct me if I'm wrong here but I believe we're going for the scenario where 
the attacker has to guess the `xxx` bits of entropy and they know that they 
have to do it. We're not actually coding `xxx` bits of entropy as we need more 
entropy bits to get a sufficient result (hence `length = 
int(math.ceil(entropy_bits / math.log(len(self.chars), 2))`).
However! To the very first question of yours - unfortunately, there is a very 
small relation between the arguments in `__init__` and `__call__` as @tiran 
says:
> I'm not clever enough to come up with an algorithm to calculate the length 
> with additional restrictions. My gut feeling tells me that less than 15% per 
> character class (3 for upper/lower case and symbols, 1 for digit) should be 
> ok.

From the code you can see that if a certain class of characters should not be 
used, it's not accounted for in the calculation of the final length of the 
password but that's about it - if a further restriction is made (>1 character 
of the give character class), this restriction is also not accounted for. But 
since we're the ones who'll be using this token generator, I think we could 
live with this. There should be a warning in a docstring somewhere, though.
"""

See the full comment at 
https://github.com/freeipa/freeipa/pull/317#issuecomment-266362288
-- 
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

[Freeipa-devel] [freeipa PR#317][comment] Unify password generation across FreeIPA

2016-12-09 Thread pspacek
  URL: https://github.com/freeipa/freeipa/pull/317
Title: #317: Unify password generation across FreeIPA

pspacek commented:
"""
@mbasti-rh You are missing the point and thus do not answer my question: The 
docstring does not tell anything about relation of 'entropy' and the output. 
What is the relation?

Does it assume that attacker knows init parameters of TokenGenerator? Or not? 
How can we do analysis without knowing threat model first? Does `entropy` mean 
that the output string simply codes `xxx` bits of entropy, or does it mean that 
attacker has to guess `xxx` bits of entropy? That should be spelled out.

I would argue that for any IPA-internal passwords we must assume that attacker 
knows the input parameters because he can easily read the source code.
"""

See the full comment at 
https://github.com/freeipa/freeipa/pull/317#issuecomment-266046041
-- 
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

[Freeipa-devel] [freeipa PR#317][comment] Unify password generation across FreeIPA

2016-12-09 Thread mbasti-rh
  URL: https://github.com/freeipa/freeipa/pull/317
Title: #317: Unify password generation across FreeIPA

mbasti-rh commented:
"""
It generates random tokens than can be used as:
- passwords
- anything else that should be random

It is written in class docstring

Yes we can randomly generate strings for each class with specified length, then 
generate randomly the rest, and finally make order of characters random. 
However I'm not sure if this is from crypto point of view equal to generate 
random string at once and then check validity, there can be something we don't 
see, but probably not.
"""

See the full comment at 
https://github.com/freeipa/freeipa/pull/317#issuecomment-266041887
-- 
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

[Freeipa-devel] [freeipa PR#317][comment] Unify password generation across FreeIPA

2016-12-09 Thread pspacek
  URL: https://github.com/freeipa/freeipa/pull/317
Title: #317: Unify password generation across FreeIPA

pspacek commented:
"""
Guys, I'm confused. What exactly is the purpose of `TokenGenerator`? The 
docstring does not explain to me what is relation between arguments in 
`__init__` and `__call__`. Of course I can guess but this should be clearly 
defined first.

If we are clever enough we can get away with the `while` loop and make it 
deterministic.
"""

See the full comment at 
https://github.com/freeipa/freeipa/pull/317#issuecomment-266030088
-- 
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

[Freeipa-devel] [freeipa PR#317][comment] Unify password generation across FreeIPA

2016-12-09 Thread mbasti-rh
  URL: https://github.com/freeipa/freeipa/pull/317
Title: #317: Unify password generation across FreeIPA

mbasti-rh commented:
"""
@tiran True, I would like to have there at least assert witch prevents devs to 
use more than let say 15% per class and more than 50% together, to prevent 
silly mistakes. It will also reduces possibility of starvation when too many 
passwords don't match requirements

Do you want to send this as PR or are you fine by adding it to this PR by 
Standa?
"""

See the full comment at 
https://github.com/freeipa/freeipa/pull/317#issuecomment-266002689
-- 
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

[Freeipa-devel] [freeipa PR#317][comment] Unify password generation across FreeIPA

2016-12-09 Thread mbasti-rh
  URL: https://github.com/freeipa/freeipa/pull/317
Title: #317: Unify password generation across FreeIPA

mbasti-rh commented:
"""
@tiran True, I would like to have there at least assert witch prevents devs to 
use more than let say 15% per class and more than 50% together, to prevent 
silly mistakes. It will also reduces possibility of starvation when too many 
password doesn't match requirements

Do you want to send this as PR or are you fine by adding it to this PR by 
Standa?
"""

See the full comment at 
https://github.com/freeipa/freeipa/pull/317#issuecomment-266002689
-- 
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

[Freeipa-devel] [freeipa PR#317][comment] Unify password generation across FreeIPA

2016-12-09 Thread tiran
  URL: https://github.com/freeipa/freeipa/pull/317
Title: #317: Unify password generation across FreeIPA

tiran commented:
"""
@mbasti-rh ```uppercase + lowercase + num + special``` should be limited to a 
sensible value. A large value invalidates the formula that calculates the 
length of the token. As far as I remember my math, formula assumes a uniform 
distribution of distinct values. Additional restrictions reduce the sample 
space of the result. I'm not clever enough to come up with an algorithm to 
calculate the length with additional restrictions. My gut feeling tells me that 
less than 15% per character class (3 for upper/lower case and symbols, 1 for 
digit) should be ok.
"""

See the full comment at 
https://github.com/freeipa/freeipa/pull/317#issuecomment-265980552
-- 
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

[Freeipa-devel] [freeipa PR#317][comment] Unify password generation across FreeIPA

2016-12-08 Thread mbasti-rh
  URL: https://github.com/freeipa/freeipa/pull/317
Title: #317: Unify password generation across FreeIPA

mbasti-rh commented:
"""
@tiran  IMO you need check `length > uppercase + lowercase + num + special`, 
otherwise infinity loop
but generally LGTM
"""

See the full comment at 
https://github.com/freeipa/freeipa/pull/317#issuecomment-265954172
-- 
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

[Freeipa-devel] [freeipa PR#317][comment] Unify password generation across FreeIPA

2016-12-08 Thread tiran
  URL: https://github.com/freeipa/freeipa/pull/317
Title: #317: Unify password generation across FreeIPA

tiran commented:
"""
```
#!/usr/bin/python3
import math
import random
import string


class TokenGenerator(object):
"""Simple, tunable token generator

TokenGenerator(uppercase=3, lowercase=3, digits=0, special=None)

At least 3 upper and 3 lower case ASCII chars, may contain digits, no
special chars.

128 bits entropy: secure
256 bits of entropy: secure enough if you care about quantum computers
"""
uppercase = frozenset(string.ascii_uppercase)
lowercase = frozenset(string.ascii_lowercase)
digits = frozenset(string.digits)
# without: = # ' " \ `
special = frozenset('!$%&()*+,-./:;<>?@[]^_{|}~')

def __init__(self, uppercase=1, lowercase=1, digits=1, special=1):
self.rng = random.SystemRandom()
self.requirements = dict(
uppercase=uppercase,
lowercase=lowercase,
digits=digits,
special=special
)
chars = set()
for symclass, req in self.requirements.items():
if req is not None:
chars.update(getattr(self, symclass))
self.chars = tuple(chars)

def __call__(self, entropy_bits=128):
length = int(math.ceil(entropy_bits / math.log(len(self.chars), 2)))
while True:
token = ''.join(self.rng.choice(self.chars) for _ in range(length))
tokenset = set(token)
token_ok = True
for symclass, req in self.requirements.items():
if req is None or req <= 0:
continue
reqchars = getattr(self, symclass)
if len(tokenset.intersection(reqchars)) < req:
token_ok = False
break
if token_ok:
return token


if __name__ == '__main__':
pwgen = TokenGenerator(special=None)
for i in range(100):
print(pwgen())
```
"""

See the full comment at 
https://github.com/freeipa/freeipa/pull/317#issuecomment-265803218
-- 
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

[Freeipa-devel] [freeipa PR#317][comment] Unify password generation across FreeIPA

2016-12-08 Thread tiran
  URL: https://github.com/freeipa/freeipa/pull/317
Title: #317: Unify password generation across FreeIPA

tiran commented:
"""
@mbasti-rh I probably misunderstood your intention. I read your comment as 
"Replace it with something sane, the sane thing is sha1".

By the way I'm currently tangled up in a twitter discussion about Python's new 
secrets module and entropy. The module doc has a nice recipe to generate 
passwords with special properties 
https://docs.python.org/3.6/library/secrets.html#recipes-and-best-practices . I 
asked a friend of mine and real (tm) cryptographer about entropy for black box 
tokens. He told me

> 128 if you don't care about quantum computing; 256 if you do

"""

See the full comment at 
https://github.com/freeipa/freeipa/pull/317#issuecomment-265789981
-- 
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

[Freeipa-devel] [freeipa PR#317][comment] Unify password generation across FreeIPA

2016-12-08 Thread mbasti-rh
  URL: https://github.com/freeipa/freeipa/pull/317
Title: #317: Unify password generation across FreeIPA

mbasti-rh commented:
"""
@tiran @simo5 If you read my comments properly I was happy with removing sha1() 
and I pointed out that ipa_generate_password() must generate entropy 160bits as 
was probably originally aimed by using sha1()

@simo5 I'm fine with removing space then

@simo5 Standa found out that when FIPS is enabled NSS is not willing to accept 
some password, it requires some special chars AFAIK @stlaz knows details

@tiran I'm afraid we need to keep special chracters there as I mentioned above ^

@tiran thank you for nice code snippet

@tiran AFAIK you misunderstood my comment, I wanted to "replace sha1 with 
something sane" or I don't understand what is wrong with my comment.
"""

See the full comment at 
https://github.com/freeipa/freeipa/pull/317#issuecomment-265786411
-- 
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

[Freeipa-devel] [freeipa PR#317][comment] Unify password generation across FreeIPA

2016-12-08 Thread tiran
  URL: https://github.com/freeipa/freeipa/pull/317
Title: #317: Unify password generation across FreeIPA

tiran commented:
"""
@stlaz Your patch looks good. My comment regarding SHA1 was aimed at comment 
https://github.com/freeipa/freeipa/pull/317#issuecomment-265440651 . The 
suggestion of SHA1 is a *Verschlimmbesserung* (improvement for the worse) of 
the current code.

I studied the implementation ```ipa_generate_password```. The special cases for 
white space makes it more complicated. If you combine @simo5 's suggestion and 
my function, you can write the function in like 6 to 7 lines of simple code. It 
might be good idea to use only alpha numeric chars, too. ```#!='"%${}.?*``` 
have special meaning in bash, C, ini files etc.
"""

See the full comment at 
https://github.com/freeipa/freeipa/pull/317#issuecomment-265766597
-- 
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

[Freeipa-devel] [freeipa PR#317][comment] Unify password generation across FreeIPA

2016-12-08 Thread simo5
  URL: https://github.com/freeipa/freeipa/pull/317
Title: #317: Unify password generation across FreeIPA

simo5 commented:
"""
We may need a max length argument if we are dealing with some stuff that has 
issues with more then max length caracters ... In that case we can warn (or 
raise, we'll have to decide) not enough entropy will be available is max length 
is not sufficient to hold the desired entropy.
"""

See the full comment at 
https://github.com/freeipa/freeipa/pull/317#issuecomment-265762543
-- 
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

[Freeipa-devel] [freeipa PR#317][comment] Unify password generation across FreeIPA

2016-12-08 Thread stlaz
  URL: https://github.com/freeipa/freeipa/pull/317
Title: #317: Unify password generation across FreeIPA

stlaz commented:
"""
@simo5 I was actually trying to get rid of SHA-1 and I am aware that entropy 
will not be raised, that part of the code draw a smile on some of our faces 
here, really :)
As for the spaces, I did not encounter issues with them in password.conf files 
which is awesome but I agree they're potentially dangerous. However, removing 
them from default set of password chars would not make our life easier as the 
check would have to stay there in case someone passes them as a possible 
character as an argument to ipa_generate_password (although they should 
probably know what they're doing, right?).
We may be able to get rid off the `characters` argument should the cases where 
it's used are found invalid though (currently in `host`, `user` passwords and 
in `dnskeysync`).
@tiran Regarding sha1 - did you see the patch? ;) However I agree that the 
length is not a good argument for password-generating function, I will have a 
look at transforming it to entropy.
"""

See the full comment at 
https://github.com/freeipa/freeipa/pull/317#issuecomment-265761543
-- 
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

[Freeipa-devel] [freeipa PR#317][comment] Unify password generation across FreeIPA

2016-12-08 Thread tiran
  URL: https://github.com/freeipa/freeipa/pull/317
Title: #317: Unify password generation across FreeIPA

tiran commented:
"""
Please don't use a hack like sha1() to turn a random byte sequence into a hex 
value. At best sha1 keeps the entropy of the input. I also don't like the fact 
that the function only cares about the length of the output. The actual length 
is irrelevant. We care about the entropy of the output.

Let's drop pwd_len and apply proper math instead:

```
import math
import random
import string

alnum = string.ascii_letters + string.digits
sysrandom = random.SystemRandom()  # uses os.urandom() as RNG

def mkpasswd(entropy_bits=128, symbols=alnum):
length = int(math.ceil(entropy_bits / math.log(len(symbols), 2)))
return ''.join(sysrandom.choice(symbols) for _ in range(length))
```
"""

See the full comment at 
https://github.com/freeipa/freeipa/pull/317#issuecomment-265760379
-- 
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

[Freeipa-devel] [freeipa PR#317][comment] Unify password generation across FreeIPA

2016-12-08 Thread simo5
  URL: https://github.com/freeipa/freeipa/pull/317
Title: #317: Unify password generation across FreeIPA

simo5 commented:
"""
@stlaz SHA-1 DOES NOT add entropy at all, you need the right number of bits in 
INPUT for whatever trasformation you use.
@mbasti-rh in what way FIPS is incompatible with base64 encoding ?
@stlaz  spaces may cause issues in some places where passwords are stored in 
files or passed (annoyingly) as shell arguments, soit is safer to avoid them in 
the final output, and given the way the code deal with space that would also 
simplify the random generator and avoid the bias on 1st and last charcter of 
the password.
"""

See the full comment at 
https://github.com/freeipa/freeipa/pull/317#issuecomment-265752256
-- 
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

[Freeipa-devel] [freeipa PR#317][comment] Unify password generation across FreeIPA

2016-12-08 Thread simo5
  URL: https://github.com/freeipa/freeipa/pull/317
Title: #317: Unify password generation across FreeIPA

simo5 commented:
"""
@stiaz, SHA-1 DOES NOT add entropy at all, you need the right number of bits in 
INPUT for whatever trasformation you use.
@mbasti-rh in what way FIPS is incompatible with base64 encoding ?
@stiaz, spaces may cause issues in some places where passwords are stored in 
files or passed (annoyingly) as shell arguments, soit is safer to avoid them in 
the final output, and given the way the code deal with space that would also 
simplify the random generator and avoid the bias on 1st and last charcter of 
the password.
"""

See the full comment at 
https://github.com/freeipa/freeipa/pull/317#issuecomment-265752256
-- 
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

[Freeipa-devel] [freeipa PR#317][comment] Unify password generation across FreeIPA

2016-12-08 Thread stlaz
  URL: https://github.com/freeipa/freeipa/pull/317
Title: #317: Unify password generation across FreeIPA

stlaz commented:
"""
Apparently, spaces are ok even in HTTP password.conf so I guess we can leave it 
there.
"""

See the full comment at 
https://github.com/freeipa/freeipa/pull/317#issuecomment-265739766
-- 
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

[Freeipa-devel] [freeipa PR#317][comment] Unify password generation across FreeIPA

2016-12-08 Thread stlaz
  URL: https://github.com/freeipa/freeipa/pull/317
Title: #317: Unify password generation across FreeIPA

stlaz commented:
"""
NSS does support spaces in its passwords it seems. My hopes are that HTTP will 
be able to understand spaces in its password.conf file.
"""

See the full comment at 
https://github.com/freeipa/freeipa/pull/317#issuecomment-265720579
-- 
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

[Freeipa-devel] [freeipa PR#317][comment] Unify password generation across FreeIPA

2016-12-08 Thread mbasti-rh
  URL: https://github.com/freeipa/freeipa/pull/317
Title: #317: Unify password generation across FreeIPA

mbasti-rh commented:
"""
> The passwords should have around the same entropy now. SHA-1 actually 
> produces 160bit outputs (hence 40-characters long hexadecimal digests), so I 
> recounted it for 20-bytes entropy.

Sure, my bad

As we discussed offline, due NSS FIPS requirements, we should get rid off 
base64 encoding. I wouldn't remove space from there, can you check if NSS 
supports it?
"""

See the full comment at 
https://github.com/freeipa/freeipa/pull/317#issuecomment-265713667
-- 
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

[Freeipa-devel] [freeipa PR#317][comment] Unify password generation across FreeIPA

2016-12-08 Thread stlaz
  URL: https://github.com/freeipa/freeipa/pull/317
Title: #317: Unify password generation across FreeIPA

stlaz commented:
"""
 The passwords should have around the same entropy now. SHA-1 actually produces 
160bit outputs (hence 40-characters long hexadecimal digests), so I recounted 
it for 20-bytes entropy.

As ipa_generate_password creates passwords of only printable characters (and a 
space) by default, base64 should not be a requirement here. However, a space 
could be a problem somewhere I guess, should it be removed as well from the 
defaul behavior of the password generator?
"""

See the full comment at 
https://github.com/freeipa/freeipa/pull/317#issuecomment-265686352
-- 
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

[Freeipa-devel] [freeipa PR#317][comment] Unify password generation across FreeIPA

2016-12-07 Thread mbasti-rh
  URL: https://github.com/freeipa/freeipa/pull/317
Title: #317: Unify password generation across FreeIPA

mbasti-rh commented:
"""
Please replace this by something sane,
```
return sha1(ipautil.ipa_generate_password()).hexdigest()
```

security by obscurity worked well in Roman empire, but now please generate 
directly password with entropy 128bits without using sha1
"""

See the full comment at 
https://github.com/freeipa/freeipa/pull/317#issuecomment-265440651
-- 
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

[Freeipa-devel] [freeipa PR#317][comment] Unify password generation across FreeIPA

2016-12-07 Thread mbasti-rh
  URL: https://github.com/freeipa/freeipa/pull/317
Title: #317: Unify password generation across FreeIPA

mbasti-rh commented:
"""
NACK

You replaced os.random() by ipa_generate_password, but ipa_generate password 
does not generate random bytes but random printable characters (entropy--) so 
you have to recalculate a new password length accordingly or edit 
ipa_generate_password function to generate random bytes.

Also I noticed you removed base64encoding, are you sure that places where it 
was used can handle all bytes characters (nonprintable, etc)? I would stay with 
base64 there.
"""

See the full comment at 
https://github.com/freeipa/freeipa/pull/317#issuecomment-265438520
-- 
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code