cloudstack-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Rajani Karuturi <Rajani.Karut...@citrix.com>
Subject Re: Findbugs report on 4.3-forward
Date Sun, 02 Feb 2014 11:48:06 GMT

I agree that we shouldn’t be using Arrays.toString() here. It would output CSV of values
enclosed in ‘[]’ (http://grepcode.com/file/repository.grepcode.com/java/root/jdk/openjdk/7u40-b43/java/util/Arrays.java/#Arrays.toString%28byte%5B%5D%29<http://grepcode.com/file/repository.grepcode.com/java/root/jdk/openjdk/7u40-b43/java/util/Arrays.java/#Arrays.toString(byte%5B%5D)>)

Like I stated in my previous mail, I think, what we should use is new String(bytes[])

what is done in the commit 2774b62d by wilderrodrigues on master (git diff 2774b62d^! -- ./plugins/user-authenticators/ldap/src/org/apache/cloudstack/api/command/LdapImportUsersCmd.java)
is the right way.
This has to be done in both the files ./plugins/user-authenticators/ldap/src/org/apache/cloudstack/api/command/LdapImportUsersCmd.java,
./plugins/user-authenticators/ldap/src/org/apache/cloudstack/api/command/LdapCreateAccountCmd.java
and in all the branches master, 4.3-forward, 4.3


regarding the encoding issue, The initial version wasn’t storing the random string directly
in db but a base64 encoded one(at least it was trying to do that and had a bug). For example,
abcdef would become YWJjZGVm. With this, it won’t pass the default plaint text encoding
authentication i.e.) you won’t be able to login with abcdef. If the encoding is known, you
can still try with encoded string.
we still have the security issue but, the new method wasn’t improving it, its just removing
one step from it. If the new method was also doing encoding maybe both of them would be equal.
Even in this case, initial version code with new String() looks cleaner to me. Though that
might vary from person to person. :)

Thanks,
~Rajani



On 02-Feb-2014, at 1:40 am, Sebastien Goasguen <runseb@gmail.com<mailto:runseb@gmail.com>>
wrote:


On Jan 31, 2014, at 12:02 PM, Ian Duffy <ian@ianduffy.ie<mailto:ian@ianduffy.ie>>
wrote:

Just continuing this for my own learning experience as I still don't see
it. I also don't see why the outputs from the commited code are OK, on
testing I'm getting things like: [55, 107, 73, 50, 87, 100, 119, 49, 121,
76, 56, 43, 104, 48, 112, 105, 107, 102, 111, 88, 87, 73, 98, 113, 120, 52,
85, 61]. I expected this to be a string made up of different characters to
form a random password, what looks like an array converted to a string.

With the char set know, it would be easy to decode the password than the
previous encoded version.


So the argument being put forward is we know the charset that makes up the
password? This is true but we know the charset for a base64 string,
its A-Z,a-z,0-9,and +. Even still, with the charset exposed I don't see how
this is an issue(bare with my math here). Its a roughly 72 charset
generating a password of length 20. Thats 72^20 different possible
combinations. If we say it takes a second to brute each combination you are

Ian, I will let Rajani and Daan reply to you, but you can get much faster than 1 combination
per second:

http://hackaday.com/2012/12/06/25-gpus-brute-force-348-billion-hashes-per-second-to-crack-your-passwords/



looking at roughly (4.445×10^29) years to test all combinations. (
http://www.wolframalpha.com/input/?i=72%5E20+seconds+to+years )

The function in question is suppose to return a string that acts as the
password given to AccountService to create the new user account.... on
testing the code that has been committed into the 4.3 branch I'm just
getting back stuff like the following: [55, 107, 73, 50, 87, 100, 119, 49,
121, 76, 56, 43, 104, 48, 112, 105, 107, 102, 111, 88, 87, 73, 98, 113,
120, 52, 85, 61]

I executed the test manually by just pulling the code out and running it
alone from command line:
https://gist.github.com/imduffy15/ae7a809aa7bb6cb198e3.

Regards,
Ian


On 31 January 2014 04:38, Rajani Karuturi <Rajani.Karuturi@citrix.com<mailto:Rajani.Karuturi@citrix.com>>wrote:

With the char set know, it would be easy to decode the password than the
previous encoded version.
This is a concern because even for ldap users we also check authentication
against db.

Thanks a lot for taking Daan and Ian.


~Rajani



On 31-Jan-2014, at 1:20 am, Ian Duffy <ian@ianduffy.ie<mailto:ian@ianduffy.ie>>
wrote:

Hi,

Sorry about the delay in replying to this have been doing exams at uni
all
week.

Daan's change looks to change Rajani concern.

Might be me being naive but I fail to understand the concern fully...
The given character selection was roughly 72 with a string of length 20.
If
my math is correct thats 72^20 different possible combinations...

Anyways, thanks for taking care of this Daan.

On 30 January 2014 13:28, Daan Hoogland <daan.hoogland@gmail.com<mailto:daan.hoogland@gmail.com>>
wrote:

Guys,

I found two reported issues of category 'scary'. To satify Rajuri's
concerns I would like to revert Ian's commit and checkin two changes
that change returning
byte[].toString()
into
Arrays.toString(byte[])
on return statements of generatePassword methods.

if no objections come in within a few...,
Daan

On Thu, Jan 30, 2014 at 1:53 PM, Daan Hoogland <daan.hoogland@gmail.com<mailto:daan.hoogland@gmail.com>

wrote:
Animesh, Ian,

Can you comment on this?

I couldn't find any findbugs issues of the the scariest kind in
yesterdays version of the 4.3 branch. What was solved that needs to go
in in spite of Rajuri's reservations?

thanks,
Daan

On Thu, Jan 30, 2014 at 11:21 AM, Daan Hoogland <
daan.hoogland@gmail.com<mailto:daan.hoogland@gmail.com>>
wrote:
Sorry Rajani,

I had seen no reaction to Ian's explenation  and the request by
Animesh to pull it so I just did. let me look into it for a minute

On Thu, Jan 30, 2014 at 5:36 AM, Rajani Karuturi
<Rajani.Karuturi@citrix.com<mailto:Rajani.Karuturi@citrix.com>> wrote:
I see that the commit 9776e1af1c92486f5081b1ee8fa95cf0edb86b97 is
already pushed to 4.3. I don't see any response on my concern as well.
Is it just me or anyone else sees a security issue with the generate
password change?
Ian/Animesh/Daan, can you please respond?

Thanks,
~Rajani



On 29-Jan-2014, at 10:59 am, Rajani Karuturi <
Rajani.Karuturi@citrix.com<mailto:Rajani.Karuturi@citrix.com>> wrote:

Hi Ian,
Before it is pushed to 4.3, can you fix the generate password change
like i suggested in the other mail? This current change would make it
less
secure.

Thanks,
~Rajani



On 29-Jan-2014, at 8:03 am, Ian Duffy <ian@ianduffy.ie<mailto:ian@ianduffy.ie>>
wrote:

Hi Animesh,

Tested all those changes to detail. Those lines were removed due to
unexpected behavior that I had not spotted until now.

They were suppose to allow for better fall over between multiple
domain
controllers, how ever they were causing caching to occur. This
meant
if a
users password was reset in LDAP the old password was still
allowing
login
for a limited time.

Please pull the changes forward,
Thanks

Ian.
On 29 Jan 2014 00:07, "Animesh Chaturvedi" <
animesh.chaturvedi@citrix.com<mailto:animesh.chaturvedi@citrix.com>>
wrote:

If I look at this commit for example




https://git-wip-us.apache.org/repos/asf?p=cloudstack.git;a=commitdiff;h=92b4f66d73562e4211d2d787554ff229dbeb5705

It removes the two lines from LdapContextFactory.java

environment.put("com.sun.jndi.ldap.read.timeout", "500");-
environment.put("com.sun.jndi.ldap.connect.pool", "true");

Is that reported by find bug? I don't know this code  so not sure
if it is
intentional or not ?

The point is there may be unintended risks in allowing late
changes.



-----Original Message-----
From: Animesh Chaturvedi [mailto:animesh.chaturvedi@citrix.com]
Sent: Tuesday, January 28, 2014 3:35 PM
To: dev@cloudstack.apache.org
Subject: RE: Findbugs report on 4.3-forward

Are you sure all of the ones are needed. A quick look at 20+
commits from
Daan  show many formatting changes that may not be necessary and
hinder
quick review.

-----Original Message-----
From: Hugo Trippaers [mailto:trippie@gmail.com]
Sent: Tuesday, January 28, 2014 3:16 PM
To: dev@cloudstack.apache.org
Subject: Re: Findbugs report on 4.3-forward



Sent from my iPhone

On 28 jan. 2014, at 23:50, Animesh Chaturvedi <
animesh.chaturvedi@citrix.com> wrote:


-----Original Message-----
From: Hugo Trippaers [mailto:trippie@gmail.com]
Sent: Tuesday, January 28, 2014 2:37 PM
To: dev@cloudstack.apache.org
Cc: dev@cloudstack.apache.org
Subject: Re: Findbugs report on 4.3-forward

Hey Animesh,

Not in agreement here. These are squashed bugs and we want as
less
bugs
in the release as possible.
[Animesh] I understand but once we enter RC phase we only limit
important fixes. I have pulled in  2 commits from yours and 1 from
Daan.

We limited our fixes to only the important issues that we found.
The other
6000 issues between coverity and findbugs are still being triaged
and will
probably not make it into this release.


This is why we test any RC before we release it.
[Animesh] Of course but timing is a bit off, if this was done a
month
back it would have been fine.
I say include all the big fixes we have in the release. If that
means
more testing before we cut the RC then that is what it is. I can't
rightfully vote for a release with known issues with existing
fixes.
[Animesh] Any release will have known issues, if we have fixes
but
can't
be sure on regression impact then we have to make a choice.

Agreed, but we just don't agree on what that choice should be yet
;-)

Quality over release schedule would be my vote then.
[Animesh] But why so late? Why was this activity not planned
early
on? I
have been reminding community to call out issues early on since
like
mid-December.

On November 4 I sent the mail to the dev list that static code
analysis
(coverity) found 6000+ issues that needed to be triaged. I worked
on quite
a few with my colleagues, but it's a big task for just the four of
us.
Findbugs just helped us to quickly identify the real scary issues
among
them.
So I agree that the timing is less than ideal, but we should do
our
utmost
best to ship the highest quality piece of software we can.


Cheers,
Hugo

Sent from my iPhone

On 28 jan. 2014, at 18:48, Animesh Chaturvedi <
animesh.chaturvedi@citrix.com> wrote:

Folks these issues reported by find-bugs have existed for some
time. I
am not confident in picking them up now for 4.3 as it may break
code that
assumed old way of working. We can take them up for 4.3
maintenance
release. I wish we had done this exercise and not waited until
now.

I will pick Hugo's commit for which he called -1.

Thanks
Animesh

-----Original Message-----
From: Trippie [mailto:trippie@gmail.com] On Behalf Of Hugo
Trippaers
Sent: Tuesday, January 28, 2014 1:29 AM
To: dev
Subject: Re: Findbugs report on 4.3-forward

Hey Animesh,

I agree with Daan here. We focussed on the bugs with a findbugs
annotation of scariest. I think that would warrant them to be
included in
the 4.3 release, so please cherry-pick them all.


Cheers,

Hugo

On 28 jan. 2014, at 09:32, Daan Hoogland <
daan.hoogland@gmail.com>
wrote:

Animesh,

I took up a lot of messages from findbugs in the server package
over
the weekend. Not that I will attach my soul to the shipping of
my
fixes but some of them are == vs eq and some are really nasty
nullpointer issues (a chack after first use is very common).
You
can
cherry-pick them or not. I don't think you should leave any of
them
behind. Even with David being right we should go against him at
every convenient time, running the risk of being called his
wife.

On Tue, Jan 28, 2014 at 6:25 AM, Ian Duffy <ian@ianduffy.ie>
wrote:
Hi Animesh,

Can you cherry-pick the below commit from from 4.3-forward to
4.3
branch?

Fix findbug issues within LDAP authenticator commit
92b4f66d73562e4211d2d787554ff229dbeb5705

Thanks,
Ian

On 28 January 2014 03:48, Animesh Chaturvedi
<animesh.chaturvedi@citrix.com>wrote:

Hugo I was reviewing your commits to 4.3-forward and looked
at
your commits



https://git-wip-us.apache.org/repos/asf?p=cloudstack.git;a=commit;
h = f18c5a1910b6370585a1d61638b8310c3ecba5ef


https://git-wip-us.apache.org/repos/asf?p=cloudstack.git;a=commit;
h = 60ac12780bfa1604902a89d5dc7937a8b9334e0d
I think you want the last one which has fixes for NetUtils
and
XenServerStorageMotionStrategy for which you had put -1 in
first
RC but the commit includes more files. Can you make limited
changes directly to 4.3? I want to build another RC later
tonight

Animesh


-----Original Message-----
From: Animesh Chaturvedi [mailto:
animesh.chaturvedi@citrix.com
]
Sent: Monday, January 27, 2014 1:30 PM
To: dev@cloudstack.apache.org
Subject: RE: Findbugs report on 4.3-forward

Agreed

We need to fix the most important ones for 4.3. There may be
assumptions in the code which we may not know and may get
broken
if these issues are fixed late. I will pull in the one Hugo
casted
his
-1 for the first vote, any others?

Animesh

-----Original Message-----
From: David Nalley [mailto:david@gnsa.us]
Sent: Monday, January 27, 2014 11:46 AM
To: dev@cloudstack.apache.org
Subject: Re: Findbugs report on 4.3-forward

So just curious if I am the only one concerned about a ton of
fixes going in at the last minute. If the fixes are for
serious
bugs and we have consensus around their severity being high
enough,
indeed lets fix things.
My concern is that much of the QA we do is manual; and while
we
are getting better; fixing tons of things at the last minute
may
have unintended consequences that we don't know about and
won't
easily find.

I yearn for the day when our automated testing is broad
enough
that we can do fixes right up to the wire and know that
things
still work, I am just not sure that I have confidence that we
are
there yet.
Thoughts? I am being paranoid?

--David

On Mon, Jan 27, 2014 at 3:11 AM, Daan Hoogland
<daan.hoogland@gmail.com>
wrote:
Animesh, I commented the once i made yesterday with
findbugs:

I allready send a few and will get you a list of the rest
later
today.

regards,

On Mon, Jan 27, 2014 at 3:48 AM, Animesh Chaturvedi
<animesh.chaturvedi@citrix.com> wrote:
Good job fellas. I see a number of commits 20+ into
4.3-forward
branch.
Are their specific commits you want me to pick up out of
these?

Animesh

-----Original Message-----
From: Daan Hoogland [mailto:daan.hoogland@gmail.com]
Sent: Sunday, January 26, 2014 2:41 AM
To: dev
Subject: Re: Findbugs report on 4.3-forward

I didn't get very far last night and will be looking at the
server
package again this afternoon.

bon appétit,

On Sun, Jan 26, 2014 at 1:36 AM, Ian Duffy <
ian@ianduffy.ie

wrote:
Hi,

Fixed the issues highlighted in the ldap user
authentication
package.

Have pushed to 4.3-forward.

Thanks,
Ian


On 25 January 2014 22:26, Daan Hoogland
<daan.hoogland@gmail.com>
wrote:

or reply to this mail with the filename you are working
on
I'll be looking at the server package as it seems to
contain
the most issues.

On Sat, Jan 25, 2014 at 4:00 PM, Hugo Trippaers
<hugo@trippaers.nl>
wrote:
I've also added a job to master with the Findbugs report
and
the
cobertura code coverage report.

Good stuff, we have a 12% coverage of our classes with
unit
tests.
Huge
improvement over the last release where we had 4% iirc.
We
have
306 reports from Findbugs, of which the majority are
internationalization
issues.
(String.getBytes without charset mostly). On the coverity
site
we have
6000+ issues still open, but at least that number is
6000+ relatively stable, we
fix as much issues as we introduce and it's untuned so we
can
assume a large number of false positives there.

I think that on average the automated tools tell us that
code
quality is
improving, which a good thing. Combined with the
functional
testing and the simulator build we can prove that we are
doing
quite well on the code quality angle.


http://jenkins.buildacloud.org/job/build-master-slowbuild/


Cheers,

Hugo


On 25 jan. 2014, at 14:13, Daan Hoogland
<daan.hoogland@gmail.com>
wrote:

H Hugo,

I'll spend some time on it tonight. Do you have a work
load
distribution scheme or is it random access?
;)

regards

On Sat, Jan 25, 2014 at 12:39 PM, Hugo Trippaers
<hugo@trippaers.nl>
wrote:
Hey all,

I've made Jenkins run the findbugs analysis on
4.3-forward.
Is there
somebody who is willing to help triage the findings?
Maybe
there is some stuff that we need to fix?

the url is

http://jenkins.buildacloud.org/job/cloudstack-4.3-forward-mave
n
-
bui
ld
/3/findbugsResult/

Cheers,

Hugo


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message