On Jul 02, 2008 at 13:56, Tomas Mandys <[EMAIL PROTECTED]> wrote:
> I've changed check_nonce() to:
>
> .....
> if (nonce->len < MIN_NONCE_LEN) {
> return 1;
> }
>
> since = get_nonce_since(nonce);
> if (since < up_since) {
> return 3;
> }
>
> expires = get_nonce_expires(nonce);
> if (calc_nonce(non, &non_len, cfg, since, expires, secret1,
> .....
>
> It causes that nonce from previous ser instance is always considered as
> stalled even it's not necessary in case when secrets are not generated,
> i.e. remain the same. I don't think it worth testing memcmp first and
> only if differs test since<up_since. I think it's marginal case and
> extra stale response is not problem. Do you agree?
I agree.
>
> If yes I'll commit it.
Please don't commit it, I already have the exact same code in my base64
version which is waiting for Jan's blessing before commit. If you commit
it, I'll run into merge conflicts :-(
Andrei
>
>
> Andrei Pelinescu-Onciul napsal(a):
> >On Jul 02, 2008 at 12:19, Tomas Mandys <[EMAIL PROTECTED]> wrote:
> >>Andrei Pelinescu-Onciul napsal(a):
> >>>Hi,
> >>>
> >>>I've just finished changing auth to use base64 nonces. However I did
> >>>find a few strange things that I want to check with you before commiting:
> >>>
> >>>1. in check_nonce(...) in the beginning we have something like:
> >>>if (get_nonce_len(cfg) != nonce->len) {
> >>> return 1; /* Lengths must be equal */
> >>>}
> >>If you need consider changing extra checks then probably yes, but if you
> >>omit this check then function never returns "1". But
> >>get_auth_checks(msg) return value is not transparent for me.
> >
> >Yes, right, there should be some form of it.
> >I think:
> > if (nonce->len<MIN_NONCE_LEN){
> > return 1; /* length musth be >= then minimum length */
> > }
> >
> >is enough. It catches nonces which are too small.
> >The get_auth_checks(msg) just tells if another MD5, appended to the end
> >of the nonce should be checked.
> >If we have a ser without auth_extra_checks restarted with
> >auth_extra_checks, then IMHO the new ser should consider a old nonce as
> >stale (and this would happen, the old nonce should be catched below by the
> >if (since < up_since)).
> >The only problems with this code might be that if auth_extra_checks are
> >turned off a longer nonce might be accepted, if it's built like :
> >valid_nonce_value + extra_stuff. But I don't think it is a problem if
> >somebody decides to add crap to a nonce (the crap part will be ignored
> >if the "required" part is ok).
> >
> >>>I think this should be deleted, because one can have for example a ser
> >>>restart with different cfg (e.g.: auth_extra_checks enabled) which might
> >>>change the nonce length. So having a different nonce lenght from the
> >>>configured one, is a valid case that should be caught by the new
> >>>up_since check.
> >>>
> >>>2. check_nonce(): the if (since < up_since) check is inside an if that
> >>>get executed only if calc_nonce(...) fails. However calc_nonce(..) fails
> >>>only if passed a nonce buffer which is too small to hold the entire
> >>>nonce (which never happens in this case).
> >>>I think it should be moved outside that if.
> >>Oaps, yes it should be put before calc_nonce() function.
> >>
> >>>3. is it ok if instead of calling is_nonce_stale() in post_auth, I call
> >>>it in check_nonce() and mark it (auth->stale=1) in auth_check_hdr_md5?
> >>>The problems is that now the nonce is a base64 encoded binary blob and I
> >>>want to decode it only once in check_nonce() and avoid decoding it a
> >>>second time in post_auth.
> >>>
> >>>
> >
> >Andrei
>
>
> --
> -----------------------------
> Tomas Mandys
> [EMAIL PROTECTED]
> Tekelec Czech Republic s.r.o.
> -----------------------------
_______________________________________________
Serdev mailing list
[email protected]
http://lists.iptel.org/mailman/listinfo/serdev
